Skip to content

Conversation

@panteliselef
Copy link
Contributor

@panteliselef panteliselef commented Aug 21, 2025

Description

Checklist

  • pnpm test runs as expected.
  • pnpm build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Summary by CodeRabbit

  • Bug Fixes
    • Made server telemetry event handling synchronous for more reliable, deterministic behavior.
  • Refactor
    • Hardened Node API access with stricter non-null checks and clearer error messaging when unavailable.
  • Chores
    • Added typed definitions for safe Node APIs to improve type safety and editor tooling.
  • Documentation
    • Recorded a patch changeset entry for the Next.js package.

@panteliselef panteliselef self-assigned this Aug 21, 2025
@vercel
Copy link

vercel bot commented Aug 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
clerk-js-sandbox Ready Ready Preview Comment Aug 21, 2025 11:52am

@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 21, 2025

Open in StackBlitz

@clerk/agent-toolkit

npm i https://pkg.pr.new/@clerk/agent-toolkit@6597

@clerk/astro

npm i https://pkg.pr.new/@clerk/astro@6597

@clerk/backend

npm i https://pkg.pr.new/@clerk/backend@6597

@clerk/chrome-extension

npm i https://pkg.pr.new/@clerk/chrome-extension@6597

@clerk/clerk-js

npm i https://pkg.pr.new/@clerk/clerk-js@6597

@clerk/dev-cli

npm i https://pkg.pr.new/@clerk/dev-cli@6597

@clerk/elements

npm i https://pkg.pr.new/@clerk/elements@6597

@clerk/clerk-expo

npm i https://pkg.pr.new/@clerk/clerk-expo@6597

@clerk/expo-passkeys

npm i https://pkg.pr.new/@clerk/expo-passkeys@6597

@clerk/express

npm i https://pkg.pr.new/@clerk/express@6597

@clerk/fastify

npm i https://pkg.pr.new/@clerk/fastify@6597

@clerk/localizations

npm i https://pkg.pr.new/@clerk/localizations@6597

@clerk/nextjs

npm i https://pkg.pr.new/@clerk/nextjs@6597

@clerk/nuxt

npm i https://pkg.pr.new/@clerk/nuxt@6597

@clerk/clerk-react

npm i https://pkg.pr.new/@clerk/clerk-react@6597

@clerk/react-router

npm i https://pkg.pr.new/@clerk/react-router@6597

@clerk/remix

npm i https://pkg.pr.new/@clerk/remix@6597

@clerk/shared

npm i https://pkg.pr.new/@clerk/shared@6597

@clerk/tanstack-react-start

npm i https://pkg.pr.new/@clerk/tanstack-react-start@6597

@clerk/testing

npm i https://pkg.pr.new/@clerk/testing@6597

@clerk/themes

npm i https://pkg.pr.new/@clerk/themes@6597

@clerk/types

npm i https://pkg.pr.new/@clerk/types@6597

@clerk/upgrade

npm i https://pkg.pr.new/@clerk/upgrade@6597

@clerk/vue

npm i https://pkg.pr.new/@clerk/vue@6597

commit: b25921d

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 21, 2025

📝 Walkthrough

Walkthrough

  • Adds a new ambient module declaration (#safe-node-apis) with typed accessors for fs, path, and cwd.
  • Refactors fs utils to introduce a generic assertNotNullable helper; replaces prior null checks.
  • Updates nodeFsOrThrow, nodePathOrThrow, and nodeCwdOrThrow to use assertNotNullable and return NonNullable types.
  • Switches keyless telemetry flag-writing from async to sync (mkdirSync/writeFileSync); adjusts call site accordingly.
  • Removes an internal throwMissingFsModule helper and a ts-ignore.
  • Adds a patch changeset entry for @clerk/nextjs describing added types for safe-node-apis.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (7)
.changeset/sharp-suits-lay.md (1)

5-5: Consider documenting the internal runtime tweak in the changeset.

This PR also switches keyless telemetry writes to synchronous I/O. Even if internal, calling that out helps consumers scanning release notes.

Apply this diff to make the note explicit:

 ---
 '@clerk/nextjs': patch
 ---
 
-Add types to safe-node-apis modules.
+Add types to safe-node-apis modules.
+
+Internal:
+- Switch keyless telemetry flag creation to synchronous FS APIs to simplify control flow.
packages/nextjs/src/types/safe-node-apis.d.ts (1)

9-16: Tighten typings: prefer optional + readonly properties and readonly export.

Using optional properties communicates intent better than | undefined, and readonly prevents accidental mutation at type level.

Apply this diff:

   interface FileSystem {
-    existsSync: typeof existsSync;
-    writeFileSync: typeof writeFileSync;
-    readFileSync: typeof readFileSync;
-    appendFileSync: typeof appendFileSync;
-    mkdirSync: typeof mkdirSync;
-    rmSync: typeof rmSync;
+    readonly existsSync: typeof existsSync;
+    readonly writeFileSync: typeof writeFileSync;
+    readonly readFileSync: typeof readFileSync;
+    readonly appendFileSync: typeof appendFileSync;
+    readonly mkdirSync: typeof mkdirSync;
+    readonly rmSync: typeof rmSync;
   }
 
   interface SafeNodeApis {
-    fs: FileSystem | undefined;
-    path: typeof nodePath | undefined;
-    cwd: (() => string) | undefined;
+    readonly fs?: FileSystem;
+    readonly path?: typeof nodePath;
+    readonly cwd?: () => string;
   }
 
-  declare const safeNodeApis: SafeNodeApis;
+  declare const safeNodeApis: Readonly<SafeNodeApis>;

Also applies to: 18-24

packages/nextjs/src/server/keyless-telemetry.ts (5)

44-44: Sync I/O: verify impact and concurrency behavior.

Switching to sync FS is simpler and fine given one-time flag creation, but it blocks the event loop on a hot path if drift detection is invoked during requests. Validate that this code runs during boot or a non-latency-critical path.

If you want non-blocking I/O without promises at the call site, consider a small worker or deferring flag creation to a background tick:

-function tryMarkTelemetryEventAsFired(): boolean {
+function tryMarkTelemetryEventAsFired(): boolean {
   try {
     if (canUseKeyless) {
       const { mkdirSync, writeFileSync } = nodeFsOrThrow();
       // ...

Optionally, gate with a cheap in-memory boolean to avoid hitting disk after the first attempt in-process.


52-53: Set explicit directory permissions (defense-in-depth).

While telemetry data here isn’t sensitive, it’s good practice to restrict perms for internal dirs.

Apply this diff:

-      mkdirSync(flagDirectory, { recursive: true });
+      mkdirSync(flagDirectory, { recursive: true, mode: 0o700 });

58-59: Specify encoding and file mode for the telemetry flag file.

Avoids platform defaults and ensures restrictive perms.

Apply this diff:

-      writeFileSync(flagFilePath, JSON.stringify(flagData, null, 2), { flag: 'wx' });
+      writeFileSync(flagFilePath, JSON.stringify(flagData, null, 2), {
+        flag: 'wx',
+        encoding: 'utf8',
+        mode: 0o600,
+      });

29-31: Optional: use the typed safe path/cwd wrappers for consistency.

This module imports path and uses process.cwd(). To stay aligned with the #safe-node-apis surface, you can switch to nodePathOrThrow() and nodeCwdOrThrow(). Not required if this file is guaranteed to be server-only.

Outside the changed lines, here is a concrete refactor sketch:

// Replace imports:
import { nodeFsOrThrow, nodePathOrThrow, nodeCwdOrThrow } from './fs/utils';

// Update implementation:
function getTelemetryFlagFilePath(): string {
  const cwd = nodeCwdOrThrow();
  const { join } = nodePathOrThrow();
  return join(cwd(), TELEMETRY_FLAG_FILE);
}

// Where dirname is needed:
const { dirname } = nodePathOrThrow();
const flagDirectory = dirname(flagFilePath);

Also applies to: 47-50


44-70: Add targeted tests for the new sync behavior.

  • Creating the flag when it doesn’t exist returns true and writes file with 0o600 perms.
  • Subsequent calls return false (EEXIST path).
  • Errors other than EEXIST log a warning and return false.

I can scaffold a test harness that stubs nodeFsOrThrow() to a temp dir FS and asserts the above without touching real disk. Want me to push a test file?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4ac7b25 and d604022.

📒 Files selected for processing (4)
  • .changeset/sharp-suits-lay.md (1 hunks)
  • packages/nextjs/src/server/fs/utils.ts (1 hunks)
  • packages/nextjs/src/server/keyless-telemetry.ts (2 hunks)
  • packages/nextjs/src/types/safe-node-apis.d.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
.changeset/**

📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)

Automated releases must use Changesets.

Files:

  • .changeset/sharp-suits-lay.md
**/*.{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/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.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/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.ts
packages/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/development.mdc)

TypeScript is required for all packages

Files:

  • packages/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.ts
packages/**/*.{ts,tsx,d.ts}

📄 CodeRabbit inference engine (.cursor/rules/development.mdc)

Packages should export TypeScript types alongside runtime code

Files:

  • packages/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.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
Avoid any type - prefer unknown when type is uncertain, then narrow with type guards
Use interface for object shapes that might be extended
Use type for unions, primitives, and computed types
Prefer readonly properties for immutable data structures
Use private for internal implementation details
Use protected for inheritance hierarchies
Use public explicitly for clarity in public APIs
Prefer readonly for 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
Use const assertions for literal types: as const
Use satisfies operator 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 ...
No any types without justification
Proper error handling with typed errors
Consistent use of readonly for 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/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.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/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.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/nextjs/src/types/safe-node-apis.d.ts
  • packages/nextjs/src/server/keyless-telemetry.ts
  • packages/nextjs/src/server/fs/utils.ts
🧬 Code graph analysis (1)
packages/nextjs/src/server/keyless-telemetry.ts (2)
packages/nextjs/src/utils/feature-flags.ts (1)
  • canUseKeyless (12-12)
packages/nextjs/src/server/fs/utils.ts (1)
  • nodeFsOrThrow (29-29)
⏰ 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). (6)
  • GitHub Check: Formatting | Dedupe | Changeset
  • GitHub Check: semgrep/ci
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (actions)
  • GitHub Check: semgrep-cloud-platform/scan
  • GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
.changeset/sharp-suits-lay.md (1)

1-6: Changeset present and scoped correctly.

Patch bump for @clerk/nextjs looks appropriate for a types-focused PR.

packages/nextjs/src/types/safe-node-apis.d.ts (2)

6-7: Good use of type-only imports.

Keeps the ambient declaration purely structural with no runtime impact.


24-26: I’m gathering your tsconfig details to confirm default-import compatibility.

packages/nextjs/src/server/keyless-telemetry.ts (2)

47-47: Correctly narrows to the safe Node FS surface.

Destructuring from nodeFsOrThrow() keeps the safe import boundary localized.


180-181: No remaining await usages for tryMarkTelemetryEventAsFired found. The call site update is correct and no further changes are needed.

packages/nextjs/src/server/fs/utils.ts (1)

14-27: Wrappers look solid and make return types explicit.

Clear NonNullable returns with a single guard keep call sites clean.

if (!nodeRuntime.cwd) {
throwMissingFsModule('cwd');
}
const nodeCwdOrThrow = (): NonNullable<typeof nodeRuntime.cwd> => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have the assertion function, what happens if we remove the explicit return type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When building @clerk/nextjs I am getting this error unless the explicit return types exist

"Exported variable 'nodeFsOrThrow' has or is using name 'FileSystem' from external module "#safe-node-apis" but cannot be named."

# Conflicts:
#	packages/nextjs/src/server/keyless-telemetry.ts
@changeset-bot
Copy link

changeset-bot bot commented Aug 21, 2025

🦋 Changeset detected

Latest commit: b25921d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clerk/nextjs Patch

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

@panteliselef panteliselef enabled auto-merge (squash) August 21, 2025 11:51
@panteliselef panteliselef merged commit 333fee7 into main Aug 21, 2025
37 checks passed
@panteliselef panteliselef deleted the elef/add-types-to-safe-fs-module branch August 21, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants