Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

fix(next-js): disable health check by defualt#1398

Closed
NathanFlurry wants to merge 1 commit into10-16-chore_core_log_headers_with__rivet_log_headersfrom
10-16-fix_next-js_disable_health_check_by_defualt
Closed

fix(next-js): disable health check by defualt#1398
NathanFlurry wants to merge 1 commit into10-16-chore_core_log_headers_with__rivet_log_headersfrom
10-16-fix_next-js_disable_health_check_by_defualt

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 16, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Oct 16, 2025 10:50am

@claude
Copy link

claude bot commented Oct 16, 2025

Claude finished @NathanFlurry's task —— View job


Pull Request Review

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the modified file (packages/next-js/src/mod.ts)
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide comprehensive feedback

Review Summary

This PR adds a single line to disable health checks by default in the Next.js integration (packages/next-js/src/mod.ts:16):

inputConfig.disableHealthCheck = true;

Code Quality & Best Practices ✅

Positive aspects:

  • The change follows the existing code style and formatting conventions
  • Includes a helpful TODO comment explaining the broader context
  • The comment clearly explains why this is needed: "Metadata endpoint will not exist at 127.0.0.1:6420"
  • Code placement is logical within the serverless configuration section
  • Variable naming follows camelCase conventions per CLAUDE.md guidelines

Minor improvements:

  • The TODO comment suggests this should eventually be moved to the registry level for all serverless runners, which shows good architectural thinking

Potential Bugs or Issues ⚠️

Concerns identified:

  1. No conditional logic: The health check is disabled unconditionally for all Next.js deployments, which may not be appropriate for all environments
  2. Missing environment consideration: Unlike other configuration (e.g., lines 19-37), this doesn't differentiate between development and production environments
  3. Hardcoded behavior: No way for users to override this behavior if they want health checks enabled

Suggested improvement:
Consider making this configurable or at least conditional based on environment:

// Only disable in serverless environments where metadata endpoint is unavailable
inputConfig.disableHealthCheck = inputConfig.runnerKind === "serverless" || process.env.RIVET_DISABLE_HEALTH_CHECK === "true";

Performance Considerations ✅

Positive impact:

  • Disabling unnecessary health checks in serverless environments will reduce startup time and eliminate failed HTTP requests
  • Removes potential timeout delays during initialization
  • Appropriate for serverless/edge environments where health check endpoints typically don't exist

Security Concerns ✅

Assessment:

  • This change reduces attack surface by disabling an endpoint that wouldn't work anyway
  • No new security vulnerabilities introduced
  • The change is defensive rather than exposing new functionality

Test Coverage ❌

Issues identified:

  1. No tests found: The packages/next-js/ directory contains no test files
  2. No integration tests: No way to verify this change works correctly in different Next.js deployment scenarios
  3. No regression protection: Future changes could accidentally re-enable health checks

Recommendations:

  • Add basic unit tests for the toNextHandler function
  • Test that disableHealthCheck is properly set to true
  • Add integration tests for different Next.js deployment environments

Additional Notes

Architecture consideration:
The TODO comment suggests moving this to the registry level for all serverless runners. This would be a good follow-up improvement to avoid duplicating this logic across different platform integrations.

Typo in PR title:
"defualt" should be "default" - consider updating the PR title for consistency.

Verdict: ✅ Approve with Suggestions

This is a sensible fix for Next.js serverless deployments. The change is minimal, well-documented, and addresses a real issue where health checks would fail in serverless environments. However, consider the suggestions above for making it more flexible and adding test coverage.


Copy link
Member Author

NathanFlurry commented Oct 16, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 16, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1398

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1398

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1398

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1398

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1398

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1398

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1398

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1398

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1398

commit: 074f8a7

@graphite-app
Copy link

graphite-app bot commented Oct 16, 2025

Merge activity

  • Oct 16, 7:43 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 16, 7:44 PM UTC: CI is running for this pull request on a draft pull request (#1403) due to your merge queue CI optimization settings.
  • Oct 16, 7:44 PM UTC: Merged by the Graphite merge queue via draft PR: #1403.

@graphite-app graphite-app bot closed this Oct 16, 2025
@graphite-app graphite-app bot deleted the 10-16-fix_next-js_disable_health_check_by_defualt branch October 16, 2025 19:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant