feat(clerk-js): Introduce debugLogger #6452
Conversation
🦋 Changeset detectedLatest commit: ce735e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 ↗︎
|
📝 WalkthroughWalkthroughAdds a debug logging subsystem to clerk-js: new types, DebugLogger class, Console/Telemetry/Composite transports, and a DebugLoggerManager with lazy/dynamic initialization and reset. Exposes a lightweight debugLogger wrapper and initDebugLogger, integrates an optional public Clerk.debugLogger and initializes debug when environment.clientDebugMode is true, emitting navigation logs via the debug logger. Extends telemetry with TelemetryLogEntry and TelemetryCollector.recordLog plus batched /v1/logs flushing. Includes unit tests for logger, telemetry transport, and telemetry batching. Also updates bundlewatch config and adds a changeset. Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
packages/types/src/json.ts (1)
74-82: Consider the necessity of property reordering.The addition of
client_debug_mode: booleanis correct and follows proper naming conventions. However, the reordering of existing properties (auth_config,user_settings,organization_settings) appears unnecessary and could potentially cause confusion without providing functional benefits.While TypeScript interfaces don't enforce property order, maintaining consistent ordering can help with code readability and reduce unnecessary diffs in version control.
packages/clerk-js/src/core/modules/debug/transports/console.ts (1)
3-32: Solid console transport implementation.The
ConsoleTransportclass provides a clean implementation of theDebugTransportinterface with proper message formatting and appropriate console method routing based on log levels.Consider adding error handling for
JSON.stringifyto prevent issues with circular references:- const context = entry.context ? ` ${JSON.stringify(entry.context)}` : ''; + const context = entry.context ? ` ${JSON.stringify(entry.context, null, 2)}` : '';Or use a safer stringify approach:
- const context = entry.context ? ` ${JSON.stringify(entry.context)}` : ''; + let context = ''; + if (entry.context) { + try { + context = ` ${JSON.stringify(entry.context)}`; + } catch (err) { + context = ` [Circular/Non-serializable context]`; + } + }packages/clerk-js/src/core/modules/debug/transports/composite.ts (2)
3-10: Interface includes unused properties.The
CompositeLoggerOptionsinterface defineslogLevelandfiltersproperties that aren't used by theCompositeTransportclass. Consider either implementing these features or removing them from the interface to avoid confusion.The
optionsproperty in the transport configuration is also unused.
12-27: Well-designed composite transport with minor logging concern.The
CompositeTransportimplementation properly handles multiple transports with good error isolation usingPromise.allSettled. However, the error logging on line 22 usesconsole.error, which could create noise or conflicts if one of the transports is aConsoleTransport.Consider using a different error handling approach or making the error logging configurable:
- console.error('Failed to send to transport:', err); + // Only log if not in browser environment or if no console transport is present + if (typeof window === 'undefined' || !this.transports.some(t => t.constructor.name === 'ConsoleTransport')) { + console.error('Failed to send to transport:', err); + }packages/clerk-js/src/core/modules/debug/transports/telemetry.ts (2)
16-22: Consider lazy timer initialization to avoid unnecessary resource usage.Starting the flush timer immediately in the constructor creates a timer even if the transport is never used for logging. Consider starting the timer on the first
send()call instead.Apply this diff for lazy timer initialization:
constructor(endpoint = 'https://telemetry.clerk.com/v1/debug', batchSize = 50, flushInterval = 10000) { this.batchSize = batchSize; this.endpoint = endpoint; this.flushInterval = flushInterval; - this.startFlushTimer(); }Then modify the
sendmethod to start the timer if not already running:async send(entry: DebugLogEntry): Promise<void> { + if (!this.flushTimer) { + this.startFlushTimer(); + } this.batchBuffer.push(entry);
32-66: Add response validation and consider retry logic for production robustness.The flush implementation correctly batches and sends data, but could be more robust for production use.
Consider adding response validation:
await fetch(this.endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(debugDataArray), -}); +}).then(response => { + if (!response.ok) { + throw new Error(`HTTP ${response.status}: ${response.statusText}`); + } +});Also consider making the
eventTypeconfigurable rather than hardcoded as'custom_event'.packages/clerk-js/src/core/clerk.ts (2)
209-216: Improve error handling and type safety.The implementation is good but could be enhanced:
- Type safety: The imported module should be type-checked
- Error logging: Consider using the existing
loggerfrom '@clerk/shared/logger' for consistency- Error details: The error message could be more specific
Apply this diff to improve the implementation:
async #initializeDebugModule(): Promise<void> { try { - const { getDebugLogger } = await import('./modules/debug'); - this.debugLogger = await getDebugLogger({}); + const debugModule = await import('./modules/debug'); + if ('getDebugLogger' in debugModule && typeof debugModule.getDebugLogger === 'function') { + this.debugLogger = await debugModule.getDebugLogger({}); + } } catch (error) { - console.error('Failed to initialize debug module:', error); + logger.warn('Failed to initialize debug module:', error); } }
858-865: Add error handling and consider access modifier.The method implementation is correct but could be improved:
- Error handling: Add try-catch around the dynamic import
- Access modifier: Consider making this method private if it's truly internal
Apply this diff to improve robustness:
/** * @internal */ - public static async __internal_resetDebugLogger(): Promise<void> { + private static async __internal_resetDebugLogger(): Promise<void> { - // This method is now handled by the debug module itself - const { __internal_resetDebugLogger } = await import('./modules/debug'); - __internal_resetDebugLogger(); + try { + const { __internal_resetDebugLogger } = await import('./modules/debug'); + __internal_resetDebugLogger(); + } catch (error) { + logger.warn('Failed to reset debug logger:', error); + } }If this method needs to be public for external testing, keep it public but add the error handling.
packages/clerk-js/src/core/modules/debug/index.ts (1)
1-171: Add comprehensive test coverage for the debug module.The coding guidelines specify that tests should be added to cover changes. This debug module needs extensive testing for:
- Singleton behavior
- Logger initialization with various options
- Error handling during initialization
- Factory function behaviors
- Reset functionality
Would you like me to generate comprehensive unit tests for this debug module to ensure proper functionality and prevent regressions?
packages/clerk-js/src/core/modules/debug/types.ts (1)
1-140: Add comprehensive test coverage for type definitions.The type guards and utility functions need test coverage to ensure they work correctly with various input scenarios.
Would you like me to generate unit tests for the type guard functions and utility types to ensure they properly validate objects and handle edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/clerk-js/src/core/clerk.ts(5 hunks)packages/clerk-js/src/core/modules/debug/index.ts(1 hunks)packages/clerk-js/src/core/modules/debug/logger.ts(1 hunks)packages/clerk-js/src/core/modules/debug/transports/composite.ts(1 hunks)packages/clerk-js/src/core/modules/debug/transports/console.ts(1 hunks)packages/clerk-js/src/core/modules/debug/transports/telemetry.ts(1 hunks)packages/clerk-js/src/core/modules/debug/types.ts(1 hunks)packages/clerk-js/src/core/resources/Environment.ts(3 hunks)packages/types/src/environment.ts(1 hunks)packages/types/src/json.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*
⚙️ CodeRabbit Configuration File
**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.Whenever reviewing a pull request, if there are any changes that could impact security, always tag
@clerk/securityin the PR.Security-impacting changes include, but are not limited to:
- Changes to authentication logic or mechanisms (e.g. login, session handling, token issuance)
- Any modification to access control, authorization checks, or role-based permissions
- Introduction or modification of hashing algorithms, signature verification, or cryptographic primitives
- Handling of sensitive data (e.g. passwords, tokens, secrets, PII)
- Integration with external identity providers (e.g. SSO, OAuth, OpenID Connect)
- Modifications to security headers, cookie flags, CORS policies, or CSRF protections
- Bypass mechanisms (e.g. feature flags, testing overrides) that could weaken protections
- Changes to rate limiting, abuse prevention, or input validation
If you're unsure whether a change is security-relevant, err on the side of caution and tag
@clerk/security.Any time that you tag
@clerk/security, please do so explicitly in a code comment, rather than within a collapsed section in a coderabbit comment, such as the "recent review details" section. If you do use the team name in any thinking or non-direct-code-comment content, it can be referred to as "clerk security team" to avoid accidentally printing the tag which sends a notification to the team.
Files:
packages/types/src/environment.tspackages/clerk-js/src/core/resources/Environment.tspackages/types/src/json.tspackages/clerk-js/src/core/modules/debug/transports/console.tspackages/clerk-js/src/core/modules/debug/transports/composite.tspackages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/transports/telemetry.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/index.{js,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use tree-shaking friendly exports
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/index.ts
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
Use index.ts files for clean imports but avoid deep barrel exports
Avoid barrel files (index.ts re-exports) as they can cause circular dependencies
Files:
packages/clerk-js/src/core/modules/debug/index.ts
🧬 Code Graph Analysis (5)
packages/types/src/json.ts (4)
packages/types/src/commerceSettings.ts (1)
CommerceSettingsJSON(6-21)packages/types/src/displayConfig.ts (1)
DisplayConfigJSON(10-48)packages/types/src/organizationSettings.ts (1)
OrganizationSettingsJSON(6-20)packages/types/src/userSettings.ts (1)
UserSettingsJSON(112-130)
packages/clerk-js/src/core/modules/debug/transports/console.ts (2)
packages/clerk-js/src/core/modules/debug/index.ts (1)
ConsoleTransport(9-9)packages/clerk-js/src/core/modules/debug/types.ts (2)
DebugTransport(43-48)DebugLogEntry(14-24)
packages/clerk-js/src/core/modules/debug/logger.ts (1)
packages/clerk-js/src/core/modules/debug/types.ts (4)
DebugTransport(43-48)DebugLogLevel(4-4)DebugLogFilter(79-86)DebugLogEntry(14-24)
packages/clerk-js/src/core/modules/debug/index.ts (6)
packages/clerk-js/src/core/modules/debug/types.ts (2)
DebugLogFilter(79-86)DebugLogLevel(4-4)packages/clerk-js/src/core/modules/debug/transports/console.ts (1)
ConsoleTransport(3-32)packages/clerk-js/src/core/modules/debug/transports/telemetry.ts (2)
TelemetryTransport(9-83)TelemetryLoggerOptions(3-7)packages/clerk-js/src/core/modules/debug/transports/composite.ts (2)
CompositeTransport(12-27)CompositeLoggerOptions(3-10)packages/clerk-js/src/core/modules/debug/logger.ts (2)
DebugLogger(6-116)error(17-19)packages/clerk-js/src/core/clerk.ts (1)
__internal_resetDebugLogger(2861-2865)
packages/clerk-js/src/core/modules/debug/transports/telemetry.ts (1)
packages/clerk-js/src/core/modules/debug/types.ts (4)
DebugLogFilter(79-86)DebugTransport(43-48)DebugLogEntry(14-24)DebugData(29-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
packages/types/src/environment.ts (1)
22-22: LGTM! Clean addition of debug mode flag.The new
clientDebugModeboolean property is appropriately named and fits well within the existingEnvironmentResourceinterface structure.packages/clerk-js/src/core/resources/Environment.ts (3)
23-23: Property declaration looks good.The
clientDebugModeproperty is properly initialized with a sensible default value offalse.
52-52: Proper initialization pattern.The use of
withDefaulthelper ensures safe initialization from JSON data, following the established pattern used by other properties in this class.
93-93: Consistent serialization.The property is correctly included in the snapshot serialization with proper snake_case naming convention for the JSON field.
packages/clerk-js/src/core/modules/debug/transports/telemetry.ts (4)
24-30: LGTM! Clean batching implementation.The batching logic is straightforward and correctly triggers a flush when the batch size is reached. The async/await usage ensures proper error propagation.
68-73: LGTM! Proper cleanup implementation.The destroy method correctly clears the timer and flushes any remaining buffered entries, preventing data loss during cleanup.
75-82: Verify browser environment compatibility.The method uses
window.setTimeoutwhich is browser-specific. Ensure this transport is only used in browser environments or consider using a more universal timer approach.The timer implementation is otherwise correct with proper error handling and recursive restart pattern.
1-83: Security review required for telemetry data transmission.This transport sends debug log entries to external endpoints, which may contain sensitive information including user IDs, session IDs, organization IDs, and arbitrary context data. The debug entries are transmitted without apparent sanitization.
@clerk/security Please review this telemetry transport implementation for:
- Potential PII leakage in debug context data
- Data sanitization requirements before external transmission
- Endpoint security and authentication requirements
- Whether debug data should be filtered before sending to telemetry
packages/clerk-js/src/core/modules/debug/logger.ts (4)
3-15: LGTM! Well-designed class with proper dependency injection.The class follows good practices with dependency injection for the transport mechanism, appropriate default values, and proper TypeScript typing.
17-35: LGTM! Consistent and clean logging method interface.All logging methods follow the same pattern with consistent signatures and proper delegation to the private log method. The API design is intuitive for developers.
37-58: Verify crypto.randomUUID() browser compatibility.The implementation is solid with proper early returns and error handling. However,
crypto.randomUUID()requires modern browser support (Chrome 92+, Firefox 95+, Safari 15.4+).Consider a fallback for older browsers or verify minimum browser requirements:
id: crypto.randomUUID?.() ?? `${Date.now()}-${Math.random().toString(36).substr(2, 9)}`,Otherwise, the logging logic and error handling are well implemented.
60-65: LGTM! Correct log level hierarchy implementation.The level checking logic correctly implements the standard logging hierarchy where error is the highest priority and trace is the lowest. The array-based approach is clean and efficient.
packages/clerk-js/src/core/clerk.ts (2)
202-207: Consider the implications of fire-and-forget async call.The debug module initialization is called without awaiting, which creates a fire-and-forget pattern. This might be intentional for lazy loading, but consider:
- Potential race conditions if
debugLoggeris accessed immediately afterupdateEnvironment- Error handling is delegated to the private method
If this behavior is intentional, consider adding a comment explaining the design decision.
1484-1484: LGTM! Good use of optional chaining for debug logging.The debug logging implementation is well done:
- Safe optional chaining prevents runtime errors
- Consistent with existing router debug pattern
- Provides valuable debugging information
packages/clerk-js/src/core/modules/debug/types.ts (3)
4-4: LGTM! Well-defined debug log levels.The debug log levels follow standard logging conventions and provide appropriate granularity for debugging purposes.
130-132: Consider if mutable utility types are necessary.The
MutableDebugLogEntryandMutableDebugDatatypes remove readonly constraints, which goes against the immutable design principle established by the readonly properties in the base interfaces.Could you clarify the use case for these mutable utility types? If they're needed for specific scenarios, consider adding JSDoc comments explaining when they should be used, as removing readonly constraints could lead to unintended mutations.
Also applies to: 137-139
14-24: LGTM! Well-structured interfaces with proper readonly properties.The interfaces are well-designed with:
- Proper use of readonly properties for immutability
- Comprehensive field coverage for debug logging needs
- Optional properties marked appropriately
- Good separation between log entries and debug data structures
Also applies to: 29-38
packages/clerk-js/src/core/modules/debug/transports/telemetry.ts
Outdated
Show resolved
Hide resolved
@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: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/clerk-js/src/core/modules/debug/index.ts (3)
58-144: Well-implemented singleton with race condition protection.The promise caching mechanism effectively prevents concurrent initialization issues, and the error handling properly clears state for retries. The implementation follows TypeScript best practices.
However, the telemetry endpoint security validation mentioned in past reviews appears to still be missing.
252-254: Add environment guard for internal reset function.As mentioned in previous reviews, this internal function should be protected with environment checks to prevent misuse in production environments.
export function __internal_resetDebugLogger(): void { + if (process.env.NODE_ENV === 'production') { + throw new Error('[Clerk] __internal_resetDebugLogger is disabled in production'); + } DebugLoggerManager.getInstance().reset(); }
106-106: Security concern: Telemetry endpoints lack validation.As mentioned in previous reviews, the telemetry endpoints are used without validation, which could enable data exfiltration to malicious endpoints. This affects multiple locations where TelemetryTransport is instantiated.
@clerk/security - Please review the telemetry endpoint configuration as it poses a data exfiltration risk.
Consider implementing endpoint validation as suggested in previous reviews:
+function isValidTelemetryEndpoint(endpoint?: string): boolean { + if (!endpoint) return true; // Use default + try { + const url = new URL(endpoint); + return url.hostname.endsWith('.clerk.com') || url.hostname === 'clerk.com'; + } catch { + return false; + } +}And validate before creating TelemetryTransport instances.
Also applies to: 172-172, 215-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clerk-js/src/core/modules/debug/index.ts(1 hunks)packages/clerk-js/src/core/modules/debug/logger.ts(1 hunks)packages/clerk-js/src/core/modules/debug/transports/console.ts(1 hunks)packages/clerk-js/src/core/modules/debug/transports/telemetry.ts(1 hunks)packages/clerk-js/src/core/modules/debug/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/src/core/modules/debug/transports/telemetry.ts
- packages/clerk-js/src/core/modules/debug/transports/console.ts
- packages/clerk-js/src/core/modules/debug/logger.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
packages/**/index.{js,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use tree-shaking friendly exports
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
**/index.ts
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
Use index.ts files for clean imports but avoid deep barrel exports
Avoid barrel files (index.ts re-exports) as they can cause circular dependencies
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*
⚙️ CodeRabbit Configuration File
**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.Whenever reviewing a pull request, if there are any changes that could impact security, always tag
@clerk/securityin the PR.Security-impacting changes include, but are not limited to:
- Changes to authentication logic or mechanisms (e.g. login, session handling, token issuance)
- Any modification to access control, authorization checks, or role-based permissions
- Introduction or modification of hashing algorithms, signature verification, or cryptographic primitives
- Handling of sensitive data (e.g. passwords, tokens, secrets, PII)
- Integration with external identity providers (e.g. SSO, OAuth, OpenID Connect)
- Modifications to security headers, cookie flags, CORS policies, or CSRF protections
- Bypass mechanisms (e.g. feature flags, testing overrides) that could weaken protections
- Changes to rate limiting, abuse prevention, or input validation
If you're unsure whether a change is security-relevant, err on the side of caution and tag
@clerk/security.Any time that you tag
@clerk/security, please do so explicitly in a code comment, rather than within a collapsed section in a coderabbit comment, such as the "recent review details" section. If you do use the team name in any thinking or non-direct-code-comment content, it can be referred to as "clerk security team" to avoid accidentally printing the tag which sends a notification to the team.
Files:
packages/clerk-js/src/core/modules/debug/index.tspackages/clerk-js/src/core/modules/debug/types.ts
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/modules/debug/index.ts (1)
packages/clerk-js/src/core/modules/debug/types.ts (4)
DebugLogLevel(4-4)isValidLogLevel(96-98)VALID_LOG_LEVELS(9-9)DebugLogFilter(84-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
packages/clerk-js/src/core/modules/debug/index.ts (3)
1-11: LGTM!Clean imports and appropriate default log level configuration.
18-25: Well-implemented validation with helpful error messaging.The generic constraint and comprehensive error message make this validation function both flexible and developer-friendly.
27-47: Excellent use of type-only exports and well-documented interfaces.Follows coding guidelines for clean imports and comprehensive JSDoc documentation.
packages/clerk-js/src/core/modules/debug/types.ts (4)
1-15: Well-defined type system with proper const assertions.The union types and readonly constant array follow TypeScript best practices for type safety.
19-91: Comprehensive interface definitions with excellent documentation.The interfaces properly use readonly properties for immutable data structures and include thorough JSDoc documentation, following the coding guidelines.
96-136: Type guards properly enhanced based on previous feedback.The type guards now correctly validate enum values against their union types, addressing the security and type safety concerns from previous reviews.
141-155: Well-implemented utility types using advanced TypeScript features.The mapped types properly handle readonly constraint removal while maintaining type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/clerk-js/src/core/modules/debug/index.ts (4)
171-172: Security concern: Same telemetry endpoint validation needed here.This function also creates a TelemetryTransport without endpoint validation, which has the same security implications as flagged in the performInitialization method.
215-215: Security concern: Same telemetry endpoint validation needed here.This function also creates a TelemetryTransport without endpoint validation, which has the same security implications as flagged in the performInitialization method.
254-256: Security concern: Internal reset function lacks environment protection.This internal function can still be called in production environments, potentially clearing important debug state. The environment guard suggested in previous reviews has not been implemented.
Add environment protection:
export function __internal_resetDebugLogger(): void { + if (typeof process !== 'undefined' && process.env?.NODE_ENV === 'production') { + throw new Error('[Clerk] __internal_resetDebugLogger is disabled in production'); + } DebugLoggerManager.getInstance().reset(); }
106-106: Security concern: Telemetry endpoint validation still missing.The telemetry endpoint is used without validation, which poses a security risk for data exfiltration. This was flagged in previous reviews but remains unaddressed.
@clerk/security - Please review the telemetry endpoint configuration as debug data could be sent to arbitrary endpoints.
Consider implementing endpoint validation:
+// Add endpoint validation helper +function isValidTelemetryEndpoint(endpoint?: string): boolean { + if (!endpoint) return true; // Use default + try { + const url = new URL(endpoint); + // Only allow clerk.com domains for security + return url.hostname.endsWith('.clerk.com') || url.hostname === 'clerk.com'; + } catch { + return false; + } +} - const transports = [{ transport: new ConsoleTransport() }, { transport: new TelemetryTransport(endpoint) }]; + if (endpoint && !isValidTelemetryEndpoint(endpoint)) { + console.warn('Invalid telemetry endpoint provided, using default'); + endpoint = undefined; + } + const transports = [{ transport: new ConsoleTransport() }, { transport: new TelemetryTransport(endpoint) }];
🧹 Nitpick comments (1)
packages/clerk-js/src/core/modules/debug/index.ts (1)
237-239: Simplify explicit type annotation.The explicit type annotation is unnecessary since TypeScript can infer the type from the CompositeLoggerOptions interface.
- const transportInstances = transports.map( - (t: { transport: DebugTransport; options?: Record<string, unknown> }) => t.transport, - ); + const transportInstances = transports.map(t => t.transport);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/clerk-js/src/core/modules/debug/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/modules/debug/index.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/modules/debug/index.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/modules/debug/index.ts
packages/**/index.{js,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use tree-shaking friendly exports
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/index.ts
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
Use index.ts files for clean imports but avoid deep barrel exports
Avoid barrel files (index.ts re-exports) as they can cause circular dependencies
Files:
packages/clerk-js/src/core/modules/debug/index.ts
**/*
⚙️ CodeRabbit Configuration File
**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.Whenever reviewing a pull request, if there are any changes that could impact security, always tag
@clerk/securityin the PR.Security-impacting changes include, but are not limited to:
- Changes to authentication logic or mechanisms (e.g. login, session handling, token issuance)
- Any modification to access control, authorization checks, or role-based permissions
- Introduction or modification of hashing algorithms, signature verification, or cryptographic primitives
- Handling of sensitive data (e.g. passwords, tokens, secrets, PII)
- Integration with external identity providers (e.g. SSO, OAuth, OpenID Connect)
- Modifications to security headers, cookie flags, CORS policies, or CSRF protections
- Bypass mechanisms (e.g. feature flags, testing overrides) that could weaken protections
- Changes to rate limiting, abuse prevention, or input validation
If you're unsure whether a change is security-relevant, err on the side of caution and tag
@clerk/security.Any time that you tag
@clerk/security, please do so explicitly in a code comment, rather than within a collapsed section in a coderabbit comment, such as the "recent review details" section. If you do use the team name in any thinking or non-direct-code-comment content, it can be referred to as "clerk security team" to avoid accidentally printing the tag which sends a notification to the team.
Files:
packages/clerk-js/src/core/modules/debug/index.ts
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/modules/debug/index.ts (6)
packages/clerk-js/src/core/modules/debug/types.ts (5)
DebugLogLevel(4-4)isValidLogLevel(96-98)VALID_LOG_LEVELS(9-9)DebugLogFilter(84-91)DebugTransport(48-53)packages/clerk-js/src/core/modules/debug/logger.ts (2)
DebugLogger(11-121)error(22-24)packages/clerk-js/src/core/modules/debug/transports/console.ts (1)
ConsoleTransport(31-69)packages/clerk-js/src/core/modules/debug/transports/telemetry.ts (2)
TelemetryTransport(9-83)TelemetryLoggerOptions(3-7)packages/clerk-js/src/core/modules/debug/transports/composite.ts (2)
CompositeTransport(12-27)CompositeLoggerOptions(3-10)packages/clerk-js/src/core/clerk.ts (1)
__internal_resetDebugLogger(2892-2896)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/clerk-js/src/core/modules/debug/index.ts (4)
1-31: LGTM! Clean imports and exports structure.The imports are well-organized with proper type-only imports where appropriate. The exports follow tree-shaking friendly patterns and properly separate runtime code from type definitions.
8-25: LGTM! Well-implemented validation with helpful error messages.The validation function uses proper generic constraints and provides clear, actionable error messages that include the invalid value and list of valid options.
32-47: LGTM! Clean interface definitions with proper documentation.The interfaces are well-structured and provide clear contracts for logger configuration options.
58-144: LGTM! Singleton pattern with proper race condition handling.The implementation correctly addresses the race condition concerns from previous reviews by using a promise cache pattern. The type safety issues have also been resolved with proper TypeScript types throughout.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/shared/src/telemetry/collector.ts (2)
102-104: LGTM! Proper type definitions for pending flush handles.The type definitions for
#logBufferand#pendingLogFlushcorrectly use union types with specific return types instead ofany, addressing the issues from previous reviews.
283-317: LGTM! Well-implemented log flush scheduling with proper concurrency control.The
#scheduleLogFlushmethod correctly implements the same flush scheduling pattern as events, with proper pending flush protection and buffer management.
🧹 Nitpick comments (4)
packages/shared/src/telemetry/collector.ts (4)
222-224: Consider implementing sampling for logs similar to events.The
#shouldRecordLogmethod currently doesn't implement sampling logic. For consistency with event handling and to control log volume, consider adding sampling support.Apply this diff to add sampling support for logs:
-#shouldRecordLog(_entry: TelemetryLogEntry): boolean { - return this.isEnabled && !this.isDebug; +#shouldRecordLog(entry: TelemetryLogEntry): boolean { + if (!this.isEnabled || this.isDebug) { + return false; + } + + // Apply sampling rate to logs if configured + if (this.#config.samplingRate < 1 && Math.random() > this.#config.samplingRate) { + return false; + } + + return true; }
254-258: Simplify cancelIdleCallback type casting.The
Number()casting forcancelIdleCallbackis unnecessary since TypeScript already knows the type from the union definition.Apply this diff to simplify the code:
if (typeof cancelIdleCallback !== 'undefined') { - cancelIdleCallback(Number(this.#pendingFlush)); + cancelIdleCallback(this.#pendingFlush as number); } else { - clearTimeout(Number(this.#pendingFlush)); + clearTimeout(this.#pendingFlush as ReturnType<typeof setTimeout>); }
343-362: Consider adding error handling and retry logic for log flushing.While silently catching errors is acceptable for telemetry, consider implementing retry logic for transient network failures to improve reliability.
Consider implementing a retry mechanism with exponential backoff for failed log submissions. This could be shared between both event and log flushing to ensure telemetry data is not lost due to temporary network issues.
Example implementation approach:
private async #flushWithRetry(url: URL, data: unknown, maxRetries = 3): Promise<void> { let lastError: Error | undefined; for (let attempt = 0; attempt < maxRetries; attempt++) { try { const response = await fetch(url, { method: 'POST', body: JSON.stringify(data), keepalive: true, headers: { 'Content-Type': 'application/json' }, }); if (response.ok) return; lastError = new Error(`HTTP ${response.status}`); } catch (error) { lastError = error as Error; } // Exponential backoff if (attempt < maxRetries - 1) { await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)); } } if (this.isDebug && lastError) { console.error('[clerk/telemetry] Failed to flush after retries:', lastError); } }
291-296: Apply same type casting simplification as in event flushing.For consistency with the event flushing code, simplify the type casting in log flush cancellation.
Apply this diff:
if (typeof cancelIdleCallback !== 'undefined') { - cancelIdleCallback(Number(this.#pendingLogFlush)); + cancelIdleCallback(this.#pendingLogFlush as number); } else { - clearTimeout(Number(this.#pendingLogFlush)); + clearTimeout(this.#pendingLogFlush as ReturnType<typeof setTimeout>); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shared/src/telemetry/collector.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/telemetry/collector.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/telemetry/collector.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/telemetry/collector.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/telemetry/collector.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/telemetry/collector.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/telemetry/collector.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/telemetry/collector.ts
🧬 Code Graph Analysis (1)
packages/shared/src/telemetry/collector.ts (3)
packages/types/src/telemetry.ts (2)
TelemetryLogEntry(49-59)TelemetryEvent(8-35)packages/clerk-js/src/core/clerk.ts (2)
sdkMetadata(298-300)sdkMetadata(302-304)packages/react/src/isomorphicClerk.ts (1)
sdkMetadata(266-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/shared/src/telemetry/collector.ts (1)
64-86: LGTM! Well-structured type definition for log data.The
TelemetryLogDatatype is properly documented and follows TypeScript best practices with clear field names and appropriate types.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/shared/src/telemetry/collector.ts (1)
311-345: LGTM! Proper concurrent flush protection implemented.The log flush scheduling correctly implements pending flush protection and handles both browser and server environments appropriately. The implementation mirrors the event flush logic with proper timer cancellation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shared/src/telemetry/collector.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/telemetry/collector.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/telemetry/collector.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/telemetry/collector.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/telemetry/collector.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/telemetry/collector.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/telemetry/collector.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/telemetry/collector.ts
🧬 Code Graph Analysis (1)
packages/shared/src/telemetry/collector.ts (4)
packages/clerk-js/src/core/modules/debug/types.ts (1)
VALID_LOG_LEVELS(9-9)packages/types/src/telemetry.ts (2)
TelemetryLogEntry(49-59)TelemetryEvent(8-35)packages/clerk-js/src/core/clerk.ts (2)
sdkMetadata(298-300)sdkMetadata(302-304)packages/react/src/isomorphicClerk.ts (1)
sdkMetadata(266-268)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/shared/src/telemetry/collector.ts (2)
464-480: LGTM! Robust context sanitization.The
#sanitizeContextmethod properly handles all edge cases and ensures JSON-serializable output. The implementation correctly rejects arrays and handles errors gracefully.
197-244: Verify and standardize error context in telemetryWe ran searches for all
recordLoginvocations but didn’t find any call sites in the repo. If you intend to userecordLogfor error reporting, please manually confirm where it’s called and ensure thecontextpayload enforces a uniform error schema. For example:• Validate that
context.erroris an object with at least
–message: string
–stack: string
–name: string
(and optional fields likecode?: string)
• Normalize or strip non-portable data from stacks (paths, timestamps)
• Apply these checks in#sanitizeContextor at the start ofrecordLogThis will help prevent inconsistent telemetry entries when errors are logged from different upstream/downstream modules.
| #shouldRecordLog(_entry: TelemetryLogEntry): boolean { | ||
| return this.isEnabled && !this.isDebug; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding rate limiting for log recording.
While event recording has throttling via TelemetryEventThrottler, the log recording doesn't have any rate limiting mechanism. This could lead to excessive log volume during error conditions or debug scenarios.
Consider implementing rate limiting similar to the event throttler:
+import { TelemetryLogThrottler } from './log-throttler';
+
export class TelemetryCollector implements TelemetryCollectorInterface {
#config: Required<TelemetryCollectorConfig>;
#eventThrottler: TelemetryEventThrottler;
+ #logThrottler: TelemetryLogThrottler;
constructor(options: TelemetryCollectorOptions) {
// ... existing code ...
this.#eventThrottler = new TelemetryEventThrottler();
+ this.#logThrottler = new TelemetryLogThrottler();
}
#shouldRecordLog(entry: TelemetryLogEntry): boolean {
- return this.isEnabled && !this.isDebug;
+ return this.isEnabled && !this.isDebug && !this.#logThrottler.isLogThrottled(entry);
}You would need to implement a TelemetryLogThrottler class similar to the existing event throttler.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/shared/src/telemetry/collector.ts around lines 250-252, log
recording currently only checks isEnabled and isDebug and lacks rate limiting;
implement a TelemetryLogThrottler (mirroring the existing
TelemetryEventThrottler) and update shouldRecordLog to consult that throttler
before allowing a log to be recorded. Create a TelemetryLogThrottler class that
exposes the same interface as the event throttler (e.g., shouldAllow(key) or
tryRecord(key)) with configurable limits and time windows, wire it into the
collector (inject or instantiate respecting existing configuration flags and
toggles), ensure it short-circuits when telemetry is disabled or in debug mode,
and add unit tests and configuration entries matching the event throttler so log
volume is controlled during error storms.
| #flushLogs(): void { | ||
| // Capture the current buffer and clear it immediately to avoid closure references | ||
| const entriesToSend = [...this.#logBuffer]; | ||
| this.#logBuffer = []; | ||
|
|
||
| this.#pendingLogFlush = null; | ||
|
|
||
| if (entriesToSend.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| fetch(new URL('/v1/logs', this.#config.endpoint), { | ||
| method: 'POST', | ||
| body: JSON.stringify(entriesToSend), | ||
| keepalive: true, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }).catch(() => void 0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and retry logic for log flushing.
The #flushLogs method silently swallows errors without any retry mechanism or error telemetry. This could lead to silent data loss during network issues.
Consider adding retry logic and error reporting:
- #flushLogs(): void {
+ async #flushLogs(retryCount = 0): Promise<void> {
// Capture the current buffer and clear it immediately to avoid closure references
const entriesToSend = [...this.#logBuffer];
this.#logBuffer = [];
this.#pendingLogFlush = null;
if (entriesToSend.length === 0) {
return;
}
- fetch(new URL('/v1/logs', this.#config.endpoint), {
- method: 'POST',
- body: JSON.stringify(entriesToSend),
- keepalive: true,
- headers: {
- 'Content-Type': 'application/json',
- },
- }).catch(() => void 0);
+ try {
+ const response = await fetch(new URL('/v1/logs', this.#config.endpoint), {
+ method: 'POST',
+ body: JSON.stringify(entriesToSend),
+ keepalive: true,
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ });
+
+ if (!response.ok && retryCount < 2) {
+ // Retry with exponential backoff
+ setTimeout(() => {
+ this.#logBuffer.unshift(...entriesToSend);
+ this.#flushLogs(retryCount + 1);
+ }, Math.pow(2, retryCount) * 1000);
+ }
+ } catch (error) {
+ if (this.isDebug) {
+ console.error('[clerk/telemetry] Failed to flush logs:', error);
+ }
+ // Consider re-queuing on recoverable errors
+ if (retryCount < 2) {
+ this.#logBuffer.unshift(...entriesToSend);
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #flushLogs(): void { | |
| // Capture the current buffer and clear it immediately to avoid closure references | |
| const entriesToSend = [...this.#logBuffer]; | |
| this.#logBuffer = []; | |
| this.#pendingLogFlush = null; | |
| if (entriesToSend.length === 0) { | |
| return; | |
| } | |
| fetch(new URL('/v1/logs', this.#config.endpoint), { | |
| method: 'POST', | |
| body: JSON.stringify(entriesToSend), | |
| keepalive: true, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }).catch(() => void 0); | |
| } | |
| async #flushLogs(retryCount = 0): Promise<void> { | |
| // Capture the current buffer and clear it immediately to avoid closure references | |
| const entriesToSend = [...this.#logBuffer]; | |
| this.#logBuffer = []; | |
| this.#pendingLogFlush = null; | |
| if (entriesToSend.length === 0) { | |
| return; | |
| } | |
| try { | |
| const response = await fetch( | |
| new URL('/v1/logs', this.#config.endpoint), | |
| { | |
| method: 'POST', | |
| body: JSON.stringify(entriesToSend), | |
| keepalive: true, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| } | |
| ); | |
| if (!response.ok && retryCount < 2) { | |
| // Retry with exponential backoff | |
| setTimeout(() => { | |
| this.#logBuffer.unshift(...entriesToSend); | |
| this.#flushLogs(retryCount + 1); | |
| }, Math.pow(2, retryCount) * 1000); | |
| } | |
| } catch (error) { | |
| if (this.isDebug) { | |
| console.error('[clerk/telemetry] Failed to flush logs:', error); | |
| } | |
| // Re-queue entries for up to 2 retries | |
| if (retryCount < 2) { | |
| this.#logBuffer.unshift(...entriesToSend); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/shared/src/telemetry/collector.ts around lines 371 to 390, the
#flushLogs implementation currently swallows fetch errors and can lose logs; add
retry and error reporting by: wrap the fetch in a retry loop with a capped
number of attempts (e.g., 3) and exponential backoff delays, on failure re-queue
the entries (append them back to this.#logBuffer or a separate retry buffer) so
they aren’t lost, and emit/record an error telemetry event or call a logger with
the error and attempt count; ensure retries are non-blocking (use setTimeout)
and don’t duplicate entries when multiple flushes run concurrently by locking or
checking a pending flag.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/shared/src/__tests__/telemetry.logs.test.ts (4)
11-11: Remove unnecessarywindowspy to reduce test fragilityThe
windowgetter spy isn’t used to drive any expectations and can be safely removed. Keeping it increases coupling to JSDOM internals without benefit.Apply this diff:
- let windowSpy: jest.SpyInstance; @@ - windowSpy = jest.spyOn(window, 'window', 'get'); @@ - windowSpy.mockRestore();Also applies to: 15-15, 19-19
38-43: Assert HTTP method and headers for stricter contract validationStrengthen the E2E assertion by checking method, headers, and keepalive. This guards against regressions in the transport.
Apply this diff after extracting
init:const [url, init] = fetchSpy.mock.calls[0]; expect(String(url)).toMatch('/v1/logs'); + expect((init as RequestInit).method).toBe('POST'); + expect((init as RequestInit).keepalive).toBe(true); + // Headers may be provided as HeadersInit; normalize to Headers for lookup + const h = new Headers((init as RequestInit).headers as HeadersInit); + expect(h.get('Content-Type')).toBe('application/json');
9-154: Add a batching test to verify multiple entries are delivered in a single POSTGiven the collector batches logs via timers, add a test to ensure two
recordLogcalls before the flush produce a single request with two entries.Proposed test to append within this
describeblock:test('batches multiple logs into a single POST', () => { const collector = new TelemetryCollector({ publishableKey: TEST_PK }); fetchSpy.mockClear(); const now = Date.now(); collector.recordLog({ id: 'id1', level: 'info', message: 'a', timestamp: now } as any); collector.recordLog({ id: 'id2', level: 'debug', message: 'b', timestamp: now } as any); jest.runAllTimers(); expect(fetchSpy).toHaveBeenCalledTimes(1); const init = fetchSpy.mock.calls[0][1] as RequestInit; expect(typeof init.body).toBe('string'); const body = JSON.parse(init.body as string); expect(Array.isArray(body)).toBe(true); expect(body).toHaveLength(2); expect(body.map((l: any) => l.iid)).toEqual(expect.arrayContaining(['id1', 'id2'])); });If you'd like, I can push this test and update snapshots accordingly.
23-53: Minor: reduceas anyusage with a local typed builderThe repeated
as anycasts can be localized via a small builder to keep tests type-safe while still allowing runtime-invalid payloads when needed.Example helper to add near the top of the file:
type AnyLog = { id: string; level: 'error' | 'warn' | 'info' | 'debug' | 'trace' | (string & {}); message: string; timestamp: number | string | any; context?: unknown; }; const makeLog = (overrides: Partial<AnyLog> = {}): AnyLog => ({ id: 'abc123', level: 'info', message: 'Hello', timestamp: Date.now(), ...overrides, });Then use
collector.recordLog(makeLog({ context: { a: 1 } }))and override fields per scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/clerk-js/bundlewatch.config.json(1 hunks)packages/shared/src/__tests__/telemetry.logs.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/bundlewatch.config.json
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/__tests__/telemetry.logs.test.ts
🪛 GitHub Check: Static analysis
packages/shared/src/__tests__/telemetry.logs.test.ts
[failure] 151-151:
'(fetchSpy.mock.calls[0][1] as RequestInit).body' may use Object's default stringification format ('[object Object]') when stringified
[failure] 85-85:
'(fetchSpy.mock.calls[0][1] as RequestInit).body' may use Object's default stringification format ('[object Object]') when stringified
[failure] 76-76:
'(fetchSpy.mock.calls[0][1] as RequestInit).body' may use Object's default stringification format ('[object Object]') when stringified
[failure] 69-69:
'(fetchSpy.mock.calls[0][1] as RequestInit).body' may use Object's default stringification format ('[object Object]') when stringified
[failure] 41-41:
'(init as RequestInit).body' may use Object's default stringification format ('[object Object]') when stringified
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
| } | ||
| } | ||
|
|
||
| #scheduleLogFlush(): void { |
There was a problem hiding this comment.
per our conversation: consider combining the event buffer here, we can have logic in flush to use a different endpoint depending on what type of event it is (telemetry event or debug log)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/clerk-js/src/core/clerk.ts (1)
481-484: LGTM: lazy-init debug module after environment is fetched.This addresses the earlier feedback to initialize after env is known and keeps the import lazy. Good placement and non-blocking initialization.
🧹 Nitpick comments (2)
packages/clerk-js/src/core/clerk.ts (2)
175-189: Remove redundant runtime type guard (or centralize it in the debug module).
getDebugLoggeralready returnsDebugLogger | null. A local structural guard adds maintenance overhead. Either:
- Drop the guard and just check for truthy, or
- Import a shared guard from the debug module (export it there if needed).
Apply this diff to remove the guard:
-/** - * Type guard to check if an object implements the DebugLoggerInterface - */ -function _isDebugLogger(obj: unknown): obj is DebugLoggerInterface { - return ( - typeof obj === 'object' && - obj !== null && - typeof (obj as DebugLoggerInterface).debug === 'function' && - typeof (obj as DebugLoggerInterface).error === 'function' && - typeof (obj as DebugLoggerInterface).info === 'function' && - typeof (obj as DebugLoggerInterface).trace === 'function' && - typeof (obj as DebugLoggerInterface).warn === 'function' - ); -}
241-241: Public API added: document with JSDoc and mark stability.This surfaces a new public property. Add JSDoc and mark it as experimental to avoid prematurely locking API surface.
- public debugLogger?: DebugLoggerInterface; + /** + * Optional debug logger instance, available when the environment enables client debug mode. + * @experimental + */ + public debugLogger?: DebugLoggerInterface;Architecture note: Consider not exposing this on the core Clerk class and instead re-export a singleton from the debug module (per prior feedback). You can still wire it internally without growing the public surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/core/clerk.ts(7 hunks)packages/shared/src/__tests__/telemetry.logs.test.ts(1 hunks)packages/shared/src/telemetry/collector.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/tests/telemetry.logs.test.ts
- packages/shared/src/telemetry/collector.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/clerk.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/clerk.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/clerk.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/clerk.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/clerk.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/clerk.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/core/clerk.ts
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/clerk.ts (3)
packages/clerk-js/src/utils/url.ts (1)
toURL(157-159)packages/clerk-js/src/core/modules/debug/index.ts (2)
getDebugLogger(155-158)__internal_resetDebugLogger(256-259)packages/clerk-js/src/core/modules/debug/logger.ts (1)
error(22-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
packages/clerk-js/src/core/clerk.ts
Outdated
| @@ -1509,6 +1539,7 @@ | |||
| const customNavigate = | |||
| options?.replace && this.#options.routerReplace ? this.#options.routerReplace : this.#options.routerPush; | |||
|
|
|||
| this.debugLogger?.info(`Clerk is navigating to: ${toURL}`); | |||
There was a problem hiding this comment.
Do not log full URLs; redact sensitive query params and use structured logging.
URLs here can include tokens (dev browser, session, handshake, etc.). Logging them risks leaking secrets to console/telemetry. Log structured, redacted values instead.
Apply this diff:
- this.debugLogger?.info(`Clerk is navigating to: ${toURL}`);
+ // Redact potentially sensitive params before logging
+ const redacted = new URL(toURL.toString());
+ ['__session','__client_uat','token','dev_browser_token','__clerk_handshake','__clerk_handshake_nonce','auth','authorization']
+ .forEach(k => {
+ if (redacted.searchParams.has(k)) {
+ redacted.searchParams.set(k, '[REDACTED]');
+ }
+ });
+ // Prefer structured logging with context and source
+ this.debugLogger?.info('navigate', { to: redacted.toString(), replace: !!options?.replace }, 'clerk.navigate');Optional: mirror the same redaction for the routerDebug console.log a few lines below to keep behavior consistent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.debugLogger?.info(`Clerk is navigating to: ${toURL}`); | |
| // Redact potentially sensitive params before logging | |
| const redacted = new URL(toURL.toString()); | |
| [ | |
| '__session', | |
| '__client_uat', | |
| 'token', | |
| 'dev_browser_token', | |
| '__clerk_handshake', | |
| '__clerk_handshake_nonce', | |
| 'auth', | |
| 'authorization' | |
| ].forEach((k) => { | |
| if (redacted.searchParams.has(k)) { | |
| redacted.searchParams.set(k, '[REDACTED]'); | |
| } | |
| }); | |
| // Prefer structured logging with context and source | |
| this.debugLogger?.info( | |
| 'navigate', | |
| { to: redacted.toString(), replace: !!options?.replace }, | |
| 'clerk.navigate' | |
| ); |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/clerk.ts around line 1542, the code logs the full
destination URL which may contain sensitive query parameters (tokens); replace
this with structured, redacted logging: parse the URL, remove or mask known
sensitive query param names (e.g., token, session, handshake, dev_browser,
etc.), and log only the safe components (e.g., pathname and a redactedQuery
object that shows keys but with values replaced by "[REDACTED]" or a boolean
indicating presence). Use the debugLogger?.info to log an object with fields
like { action: 'navigate', path: <pathname>, query: <redactedQuery>,
originalHost: <host> } rather than the full URL string; apply the same redaction
and structured logging to the nearby routerDebug console.log for consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/clerk-js/src/core/clerk.ts (3)
223-225: Avoid expanding the public surface with a debugLogger property; rely on the singleton.The comment says “Deprecated: use global singleton,” yet this adds a new public property. Prefer not introducing it at all to keep the API surface stable. If exposure is needed later, add it intentionally with docs.
Apply this diff to remove the property:
- // Deprecated: use global singleton from `@/utils/debug` - public debugLogger?: DebugLoggerInterface;If you must retain it, mark as internal:
- // Deprecated: use global singleton from `@/utils/debug` - public debugLogger?: DebugLoggerInterface; + /** + * @internal Prefer the `debugLogger` singleton from `@/utils/debug` + */ + public debugLogger?: DebugLoggerInterface;If consumers need logging, recommend: import { debugLogger } from '@/utils/debug'.
2915-2922: Harden reset: handle errors and clear exposed references.Wrap dynamic import in try/catch and clear any public references to avoid stale handles after reset. Prefer using shared logger for warnings.
Apply this diff:
/** * @internal */ public static async __internal_resetDebugLogger(): Promise<void> { - // This method is now handled by the debug module itself - const { __internal_resetDebugLogger } = await import('./modules/debug'); - __internal_resetDebugLogger(); + try { + const { __internal_resetDebugLogger } = await import('./modules/debug'); + __internal_resetDebugLogger(); + if (typeof window !== 'undefined' && window.Clerk) { + // Clear any exposed reference + window.Clerk.debugLogger = undefined as unknown as DebugLoggerInterface; + } + } catch (e) { + logger.warnOnce('Clerk: failed to reset debug logger'); + } }
1529-1532: Do not log full URLs; redact sensitive params and use structured logging.Query params may include secrets (session, tokens, handshake). Log a redacted URL and use structured logging.
Apply this diff:
- debugLogger.info(`Clerk is navigating to: ${toURL}`); - if (this.#options.routerDebug) { - console.log(`Clerk is navigating to: ${toURL}`); - } + // Redact potentially sensitive params before logging + const redacted = new URL(toURL.toString()); + ['__session','__client_uat','token','dev_browser_token','__clerk_handshake','__clerk_handshake_nonce','auth','authorization'] + .forEach(k => { + if (redacted.searchParams.has(k)) { + redacted.searchParams.set(k, '[REDACTED]'); + } + }); + debugLogger.info('navigate', { to: redacted.toString(), replace: !!options?.replace }, 'clerk.navigate'); + if (this.#options.routerDebug) { + console.log(`Clerk is navigating to: ${redacted}`); + }
🧹 Nitpick comments (6)
packages/clerk-js/src/core/modules/debug/logger.ts (3)
65-70: Avoid re-allocating level ordering array on every call.Hoist the levels array to module scope (const assertion) to reduce per-call allocations and improve readability.
Apply this diff:
- private shouldLogLevel(level: DebugLogLevel): boolean { - const levels: DebugLogLevel[] = ['error', 'warn', 'info', 'debug', 'trace']; + private shouldLogLevel(level: DebugLogLevel): boolean { + const levels = LOG_LEVELS; const currentLevelIndex = levels.indexOf(this.logLevel); const messageLevelIndex = levels.indexOf(level); return messageLevelIndex <= currentLevelIndex; }And add near the top of the file:
const LOG_LEVELS = ['error', 'warn', 'info', 'debug', 'trace'] as const satisfies readonly DebugLogLevel[];
22-40: Make public APIs explicit and add concise JSDoc to methods.For consistency with the codebase guidelines, mark methods as public and add brief JSDoc on parameters and source usage.
Apply this diff:
- debug(message: string, context?: Record<string, unknown>, source?: string): void { + /** Log at debug level. `source` should be a stable identifier (e.g., "clerk.navigate"). */ + public debug(message: string, context?: Record<string, unknown>, source?: string): void { this.log('debug', message, context, source); } - error(message: string, context?: Record<string, unknown>, source?: string): void { + /** Log at error level. */ + public error(message: string, context?: Record<string, unknown>, source?: string): void { this.log('error', message, context, source); } - info(message: string, context?: Record<string, unknown>, source?: string): void { + /** Log at info level. */ + public info(message: string, context?: Record<string, unknown>, source?: string): void { this.log('info', message, context, source); } - warn(message: string, context?: Record<string, unknown>, source?: string): void { + /** Log at warn level. */ + public warn(message: string, context?: Record<string, unknown>, source?: string): void { this.log('warn', message, context, source); } - trace(message: string, context?: Record<string, unknown>, source?: string): void { + /** Log at trace level. */ + public trace(message: string, context?: Record<string, unknown>, source?: string): void { this.log('trace', message, context, source); }
60-62: Avoid console.error in core path; prefer a guarded fallback or shared logger.If the active transport is ConsoleTransport and throws, directly logging to console here can get noisy. Consider using a minimal guarded fallback or the shared logger (if no circular deps).
Possible minimal tweak:
- this.transport.send(entry).catch(err => { - console.error('Failed to send log entry:', err); - }); + this.transport.send(entry).catch(err => { + // Fallback error path; avoid throwing and avoid recursion. + if (typeof console !== 'undefined' && typeof console.error === 'function') { + console.error('Clerk debug transport failed to send log entry', err); + } + });packages/clerk-js/src/core/clerk.ts (1)
464-471: Initialize debug mode without forcing the noisiest level.Hardcoding logLevel: 'trace' can be very noisy. Let the debug module decide defaults or derive from environment/user options. You can omit logLevel here.
Apply this diff:
- if (this.environment?.clientDebugMode) { - initDebugLogger({ - enabled: true, - logLevel: 'trace', - telemetryCollector: this.telemetry, - }); - } + if (this.environment?.clientDebugMode) { + initDebugLogger({ + enabled: true, + telemetryCollector: this.telemetry, + }); + }packages/clerk-js/src/utils/debug.ts (2)
17-22: Export InitOptions for external consumers and add brief JSDoc.Exporting InitOptions helps callers type their config and aligns with the guideline to export types alongside runtime code.
Apply this diff:
-type InitOptions = { +/** Options for initializing the debug logger singleton. */ +export type InitOptions = { enabled?: boolean; filters?: DebugLogFilter[]; logLevel?: DebugLogLevel; telemetryCollector?: TelemetryCollector; };
9-15: Consider documenting thesourceparameter contract.Documenting expected values for source (stable identifiers) improves discoverability and filtering consistency across the codebase.
Proposed JSDoc:
/** * Lightweight logger surface that callers can import as a singleton. * - Methods are no-ops until initialized via `initDebugLogger`. * - `source` should be a stable identifier (e.g., "clerk.navigate", "auth.web3"). */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/core/clerk.ts(7 hunks)packages/clerk-js/src/core/modules/debug/logger.ts(1 hunks)packages/clerk-js/src/utils/debug.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/core/modules/debug/logger.tspackages/clerk-js/src/utils/debug.tspackages/clerk-js/src/core/clerk.ts
🧬 Code Graph Analysis (3)
packages/clerk-js/src/core/modules/debug/logger.ts (1)
packages/clerk-js/src/core/modules/debug/types.ts (4)
DebugLogLevel(4-4)DebugTransport(48-53)DebugLogFilter(84-91)DebugLogEntry(19-29)
packages/clerk-js/src/utils/debug.ts (2)
packages/clerk-js/src/core/modules/debug/types.ts (2)
DebugLogFilter(84-91)DebugLogLevel(4-4)packages/clerk-js/src/core/modules/debug/index.ts (1)
getDebugLogger(155-158)
packages/clerk-js/src/core/clerk.ts (3)
packages/clerk-js/src/utils/debug.ts (3)
DebugLoggerInterface(9-15)initDebugLogger(47-58)debugLogger(60-91)packages/clerk-js/src/utils/url.ts (1)
toURL(157-159)packages/clerk-js/src/core/modules/debug/index.ts (1)
__internal_resetDebugLogger(256-259)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
packages/clerk-js/src/core/modules/debug/logger.ts (1)
72-104: Filters ignore sessionId/userId/organizationId fields; verify intended enrichment path.DebugLogFilter includes sessionId/userId/organizationId, but they’re never considered here, and DebugLogEntry is emitted without these fields. If enrichment is expected in transports/manager, fine; otherwise filters targeting these fields will never match.
If enrichment is not happening elsewhere, extend filtering to include these fields from context and/or add them to the emitted entry. For example:
- Accept context in shouldLogFilters and check context.userId/sessionId/organizationId when corresponding filter fields are present.
- Optionally, propagate known identifiers to the entry:
- private log(level: DebugLogLevel, message: string, context?: Record<string, unknown>, source?: string): void { + private log(level: DebugLogLevel, message: string, context?: Record<string, unknown>, source?: string): void { if (!this.shouldLogLevel(level)) { return; } - if (!this.shouldLogFilters(level, message, source)) { + if (!this.shouldLogFilters(level, message, source, context)) { return; } const entry: DebugLogEntry = { id: safeRandomUUID(), timestamp: Date.now(), level, message, context, source, + userId: (context?.['userId'] as string | undefined), + sessionId: (context?.['sessionId'] as string | undefined), + organizationId: (context?.['organizationId'] as string | undefined), };And update shouldLogFilters signature to also consider these fields.
packages/clerk-js/src/core/clerk.ts (1)
89-91: LGTM: Type-only import and singleton debug surface wired correctly.Using a type-only import for DebugLoggerInterface and importing the singleton debugLogger/initDebugLogger follows the intended design.
packages/clerk-js/src/utils/debug.ts (2)
60-91: LGTM: No-op wrapper defers to real logger safely.The wrapper methods are minimal, defensive, and safe to call before initialization.
1-4: Type-only imports: good. Verify path alias resolution at build time.Using '@/core/modules/debug/types' relies on TS path aliases; ensure the package tsconfig maps '@/*' correctly for bundlers and type builds.
If needed, I can generate a quick script to grep for the tsconfig path mappings to confirm alignment across packages.
| export function initDebugLogger(options: InitOptions = {}): void { | ||
| const { enabled = true, ...rest } = options; | ||
| lastOptions = rest; | ||
| isEnabled = Boolean(enabled); | ||
|
|
||
| if (!isEnabled) { | ||
| realLogger = null; | ||
| return; | ||
| } | ||
|
|
||
| void ensureInitialized(); | ||
| } |
There was a problem hiding this comment.
Subsequent initDebugLogger calls don’t reconfigure an existing logger (docstring is inaccurate).
Currently, once realLogger is set, re-invocations only update lastOptions; the existing instance is never reconfigured. Either reinitialize via manager or provide a configure path.
Apply this diff to reconfigure when enabled and a logger already exists:
export function initDebugLogger(options: InitOptions = {}): void {
const { enabled = true, ...rest } = options;
lastOptions = rest;
isEnabled = Boolean(enabled);
if (!isEnabled) {
realLogger = null;
return;
}
- void ensureInitialized();
+ // If already initialized, reconfigure via the manager
+ if (realLogger) {
+ void (async () => {
+ try {
+ const { getDebugLogger } = await import('@/core/modules/debug');
+ const updated = await getDebugLogger({
+ filters: lastOptions?.filters,
+ logLevel: lastOptions?.logLevel ?? 'trace',
+ telemetryCollector: lastOptions?.telemetryCollector,
+ });
+ if (updated) {
+ realLogger = updated;
+ }
+ } catch {
+ // Swallow to avoid breaking callers
+ }
+ })();
+ return;
+ }
+
+ void ensureInitialized();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function initDebugLogger(options: InitOptions = {}): void { | |
| const { enabled = true, ...rest } = options; | |
| lastOptions = rest; | |
| isEnabled = Boolean(enabled); | |
| if (!isEnabled) { | |
| realLogger = null; | |
| return; | |
| } | |
| void ensureInitialized(); | |
| } | |
| export function initDebugLogger(options: InitOptions = {}): void { | |
| const { enabled = true, ...rest } = options; | |
| lastOptions = rest; | |
| isEnabled = Boolean(enabled); | |
| if (!isEnabled) { | |
| realLogger = null; | |
| return; | |
| } | |
| // If already initialized, reconfigure via the manager | |
| if (realLogger) { | |
| void (async () => { | |
| try { | |
| const { getDebugLogger } = await import('@/core/modules/debug'); | |
| const updated = await getDebugLogger({ | |
| filters: lastOptions?.filters, | |
| logLevel: lastOptions?.logLevel ?? 'trace', | |
| telemetryCollector: lastOptions?.telemetryCollector, | |
| }); | |
| if (updated) { | |
| realLogger = updated; | |
| } | |
| } catch { | |
| // Swallow to avoid breaking callers | |
| } | |
| })(); | |
| return; | |
| } | |
| void ensureInitialized(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shared/src/utils/uuid.ts (2)
1-8: Clarify JSDoc: security note and @returnsSince this is a public util and may be reused broadly, make the fallback’s non-cryptographic nature explicit and document the return format.
/** * Generates a RFC 4122 v4 UUID using the best available source of randomness. * * Order of preference: * - crypto.randomUUID (when available) * - crypto.getRandomValues with manual v4 formatting * - Math.random-based fallback (not cryptographically secure; last resort) + * + * Note: In environments without Web Crypto, this function uses Math.random + * and MUST NOT be used for security-sensitive identifiers. + * + * @returns A string in RFC 4122 v4 format (8-4-4-4-12). */
53-74: Add a unit test for the fallback pathPlease add a test that temporarily disables globalThis.crypto to force the fallback and asserts the result matches:
- Regex: /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i
- Length: 36
Also restore globalThis.crypto after the test to avoid cross-test pollution. I can draft this test if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clerk-js/src/core/clerk.ts(6 hunks)packages/clerk-js/src/core/modules/debug/index.ts(1 hunks)packages/clerk-js/src/core/modules/debug/logger.ts(1 hunks)packages/shared/src/utils/index.ts(1 hunks)packages/shared/src/utils/uuid.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/src/core/modules/debug/index.ts
- packages/clerk-js/src/core/modules/debug/logger.ts
- packages/clerk-js/src/core/clerk.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
packages/**/index.{js,ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use tree-shaking friendly exports
Files:
packages/shared/src/utils/index.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
**/index.ts
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
Use index.ts files for clean imports but avoid deep barrel exports
Avoid barrel files (index.ts re-exports) as they can cause circular dependencies
Files:
packages/shared/src/utils/index.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/uuid.ts
🧬 Code Graph Analysis (1)
packages/shared/src/utils/uuid.ts (1)
packages/shared/src/utils/index.ts (1)
generateUuid(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/shared/src/utils/index.ts (1)
9-9: Named re-export looks good and is tree-shaking friendlyThe public re-export of generateUuid via the utils barrel is consistent with existing patterns in this module and enables ergonomic imports.
packages/shared/src/utils/uuid.ts (1)
20-23: Version and variant bits are set correctlyMasking and setting the version (4) and variant (10) bits is correct per RFC 4122 §4.4. Good job.
| export function generateUuid(): string { | ||
| const cryptoApi = (globalThis as unknown as { crypto?: Crypto }).crypto; | ||
|
|
||
| if (cryptoApi && typeof (cryptoApi as any).randomUUID === 'function') { | ||
| return (cryptoApi as any).randomUUID(); | ||
| } | ||
|
|
||
| if (cryptoApi && typeof cryptoApi.getRandomValues === 'function') { | ||
| const bytes = new Uint8Array(16); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make crypto detection type-safe across non-DOM TS targets; remove reliance on DOM Crypto type
Referencing the DOM Crypto type can fail compilation in packages that don’t include DOM libs. Also, avoid as any where we can keep types precise.
Apply this diff locally, then add the type alias below:
export function generateUuid(): string {
- const cryptoApi = (globalThis as unknown as { crypto?: Crypto }).crypto;
-
- if (cryptoApi && typeof (cryptoApi as any).randomUUID === 'function') {
- return (cryptoApi as any).randomUUID();
- }
+ const cryptoApi = (globalThis as any).crypto as CryptoLike | undefined;
+ if (cryptoApi?.randomUUID && typeof cryptoApi.randomUUID === 'function') {
+ return cryptoApi.randomUUID();
+ }
- if (cryptoApi && typeof cryptoApi.getRandomValues === 'function') {
+ if (cryptoApi?.getRandomValues && typeof cryptoApi.getRandomValues === 'function') {Add this type alias near the top of the file (outside the selected range):
type CryptoLike = {
randomUUID?: () => string;
getRandomValues?: (b: Uint8Array) => Uint8Array;
};🤖 Prompt for AI Agents
In packages/shared/src/utils/uuid.ts around lines 9 to 17, the code relies on
the DOM Crypto type and uses coarse any casts which breaks compilation for
non-DOM TS targets; add a local type alias named CryptoLike (with optional
randomUUID and getRandomValues signatures) near the top of the file (outside the
selected range) and then change the cryptoApi declaration to use that CryptoLike
type instead of the DOM Crypto, removing the `as any` casts and performing
type-safe checks for randomUUID and getRandomValues before calling them.
| // Last-resort fallback for very old environments | ||
| const fallback = () => | ||
| Math.floor((1 + Math.random()) * 0x10000) | ||
| .toString(16) | ||
| .substring(1); | ||
| return ( | ||
| fallback() + | ||
| fallback() + | ||
| fallback() + | ||
| fallback() + | ||
| '-' + | ||
| fallback() + | ||
| '-' + | ||
| fallback() + | ||
| '-' + | ||
| fallback() + | ||
| '-' + | ||
| fallback() + | ||
| fallback() + | ||
| fallback() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix fallback: invalid UUID format and missing RFC 4122 v4 guarantees
The Math.random fallback currently emits a 16-4-4-4-12 pattern (should be 8-4-4-4-12) and doesn’t enforce version/variant bits. This violates RFC 4122 and can break downstream consumers validating UUIDs.
Apply this diff to implement a correct RFC 4122 v4-compatible fallback:
- // Last-resort fallback for very old environments
- const fallback = () =>
- Math.floor((1 + Math.random()) * 0x10000)
- .toString(16)
- .substring(1);
- return (
- fallback() +
- fallback() +
- fallback() +
- fallback() +
- '-' +
- fallback() +
- '-' +
- fallback() +
- '-' +
- fallback() +
- '-' +
- fallback() +
- fallback() +
- fallback()
- );
+ // Last-resort fallback for very old environments (not cryptographically secure)
+ // Format: 8-4-4-4-12, with version=4 and variant=8|9|a|b
+ return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, c => {
+ const r = Math.floor(Math.random() * 16);
+ const v = c === 'x' ? r : (r & 0x3) | 0x8;
+ return v.toString(16);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Last-resort fallback for very old environments | |
| const fallback = () => | |
| Math.floor((1 + Math.random()) * 0x10000) | |
| .toString(16) | |
| .substring(1); | |
| return ( | |
| fallback() + | |
| fallback() + | |
| fallback() + | |
| fallback() + | |
| '-' + | |
| fallback() + | |
| '-' + | |
| fallback() + | |
| '-' + | |
| fallback() + | |
| '-' + | |
| fallback() + | |
| fallback() + | |
| fallback() | |
| ); | |
| } | |
| // Last-resort fallback for very old environments (not cryptographically secure) | |
| // Format: 8-4-4-4-12, with version=4 and variant=8|9|a|b | |
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, c => { | |
| const r = Math.floor(Math.random() * 16); | |
| const v = c === 'x' ? r : (r & 0x3) | 0x8; | |
| return v.toString(16); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/shared/src/utils/uuid.ts around lines 53 to 74, the fallback UUID
generator produces the wrong 16-4-4-4-12 segment layout and does not set RFC
4122 v4 version/variant bits; replace the fallback with logic that: generates 16
random bytes using Math.random when crypto is unavailable, sets bytes[6] =
(bytes[6] & 0x0f) | 0x40 to enforce version 4, sets bytes[8] = (bytes[8] & 0x3f)
| 0x80 to enforce the variant, converts the bytes to hex pairs, and returns a
formatted UUID string in the correct 8-4-4-4-12 pattern.
Description
Create a debugLogger module that will be lazy instantiated when environment shows we are in a client debug mode.
Fixes: USER-2540
Related: https://github.com/clerk/cloudflare-workers/pull/672
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores