-
Notifications
You must be signed in to change notification settings - Fork 370
chore(shared): Track usage of react hooks #6158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: dc253a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
adfe3a1
to
dc253a2
Compare
📝 WalkthroughWalkthroughThe changes introduce telemetry event recording to several React hooks in the shared package, specifically 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shared/src/react/hooks/useSessionList.ts (1)
51-57
: Eliminate redundant clerk instance variable.The hook now has two variables from
useClerkInstanceContext()
:isomorphicClerk
(line 53) andclerk
(line 55). This creates unnecessary redundancy.Use the existing
isomorphicClerk
variable for telemetry instead:export const useSessionList = (): UseSessionListReturn => { useAssertWrappedByClerkProvider(hookName); const isomorphicClerk = useClerkInstanceContext(); const client = useClientContext(); - const clerk = useClerkInstanceContext(); - clerk.telemetry?.record(eventMethodCalled(hookName)); + isomorphicClerk.telemetry?.record(eventMethodCalled(hookName));packages/shared/src/react/hooks/useSession.ts (1)
8-8
: Minor: Consider using single quotes for consistency.While functionally identical, other hooks use single quotes for the
hookName
constant. Consider using'useSession'
instead of template literals for consistency.-const hookName = `useSession`; +const hookName = 'useSession';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/fresh-regions-stand.md
(1 hunks)packages/shared/src/react/hooks/useReverification.ts
(1 hunks)packages/shared/src/react/hooks/useSession.ts
(2 hunks)packages/shared/src/react/hooks/useSessionList.ts
(2 hunks)packages/shared/src/react/hooks/useUser.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
.changeset/fresh-regions-stand.md (1)
1-6
: LGTM! Changeset properly documents the telemetry tracking changes.The changeset correctly identifies this as a patch update to
@clerk/shared
and accurately describes the changes being made to track telemetry of hooks.packages/shared/src/react/hooks/useUser.ts (3)
3-4
: LGTM! Proper telemetry imports and context usage.The telemetry imports and
useClerkInstanceContext
addition are correctly structured and align with the telemetry tracking objectives.
6-6
: LGTM! Good practice using a constant for the hook name.Using
hookName
constant instead of hardcoded strings improves maintainability and consistency across the codebase.
131-136
: LGTM! Telemetry implementation follows best practices.The telemetry recording is properly implemented:
- Uses the hook name constant for consistency
- Records immediately after getting the clerk instance
- Uses optional chaining for safety
- Doesn't interfere with the core hook logic
packages/shared/src/react/hooks/useSessionList.ts (1)
3-6
: LGTM! Proper telemetry imports and hook name constant.The telemetry imports and
hookName
constant follow the established pattern consistently.packages/shared/src/react/hooks/useSession.ts (2)
3-4
: LGTM! Proper telemetry imports and context usage.The telemetry imports and context modifications are correctly implemented.
59-64
: LGTM! Telemetry implementation is correct.The telemetry recording follows the established pattern and is properly positioned before the core hook logic.
packages/shared/src/react/hooks/useReverification.ts (1)
201-205
: LGTM! Appropriate telemetry implementation with valuable metadata.The telemetry recording is well-implemented for this hook:
- Uses existing
useClerk()
hook appropriately- Records valuable metadata about
onNeedsReverification
usage- Positioned correctly to capture all hook invocations
- Follows the pattern established in other hooks while adapting to this hook's specific context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/violet-symbols-go.md (1)
5-5
: Capitalize "React"
The framework name should be capitalized as a proper noun.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/violet-symbols-go.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/violet-symbols-go.md
[grammar] ~5-~5: “React” is a proper noun and needs to be capitalized.
Context: ...lerk/shared': patch --- Track usage of react hooks on development instances. - `useR...
(A_GOOGLE)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
Description
Reminder that this is applied only to dev instances.
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change
Summary by CodeRabbit