fix(rivetkit/next-js): fix request handler

[NathanFlurry]

No description provided.

[NathanFlurry]

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

• fix(rivetkit/next-js): fix request handler #4001 👈 (View in Graphite)
• fix(rivetkit): handle undefined options with scheduling #4000
• chore(engine): add force shutdown timeout #3999
• fix(rivetkit): fix fetch handler in nextjs serverless #3998
• fix(rivetkit): add back dev hot reloading in next #3997
• fix(rivetkit): fix routing of /websocket #3995
• fix(rivetkit): add config parameters to spawned serverless engine for local dev #3994
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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]

More templates

• actor-actions
• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• cross-actor-actions
• example-cursors
• example-cursors-raw-websocket
• custom-serverless
• example-sqlite
• example-elysia
• example-experimental-durable-streams-ai-agent
• hello-world
• example-hono
• hono-react
• example-kitchen-sink
• multi-region
• native-websockets
• next-js
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• scheduling
• state
• example-stream
• example-trpc

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4001

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4001

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4001

commit: ce43ed9

[claude]

PR Review: fix(rivetkit/next-js): fix request handler

Summary

This PR refactors the Next.js request handler in RivetKit, improving code organization, error handling, and development experience. The changes include better request handling, improved file watching for hot reloading, and a new dev environment script.

Code Quality & Best Practices

✅ Strengths

1. Better Code Organization

   • Extracted constants (ROUTE_FILE, WATCH_INTERVAL_MS) to the top of the file for better maintainability
   • Created cloneRequestWithSignal() helper function to simplify request cloning logic
   • Improved separation of concerns in handleRequestWithFileWatcher()

2. Improved Error Handling

   • Added try-catch block around registry.handler() call with fallback to direct handler (lines 116-126)
   • Better handling of missing route files during development with warning suppression after first warning

3. Better Developer Experience

   • Added DELETE method support (line 76)
   • Reduced watch interval from 1000ms to 500ms for faster hot reload detection
   • Improved logging with more descriptive messages using structured logging
   • New dev-env.sh script provides sensible defaults for local development

4. Code Clarity

   • Removed unnecessary comments that explained obvious code behavior
   • Consistent variable naming (newUrl → targetUrl)
   • Simplified conditional logic (changed === "development" to !== "production")

⚠️ Potential Issues

1. Request Signal Handling (rivetkit-typescript/packages/next-js/src/mod.ts:98)

   • Direct property access request.signal.addEventListener could throw if request.signal is null/undefined
   • Recommendation: Add null check or use optional chaining:

   request.signal?.addEventListener("abort", abortMerged);

2. Double Event Listener Registration (lines 98, 104-107)

   • The abortMerged callback is registered at line 98
   • A second abort listener is registered at lines 104-107 that calls clearWatcher()
   • Issue: Both listeners will fire on abort, but the second one seems redundant since clearWatcher() will be called when the watcher detects the abort
   • Recommendation: Consider if both listeners are needed or consolidate them

3. Missing Event Listener Cleanup (line 98)

   • The abortMerged listener is added but never removed, which could cause memory leaks in long-running processes
   • Recommendation: Store the listener reference and remove it in cleanup:

   try {
     request.signal?.addEventListener("abort", abortMerged);
     // ... rest of logic
   } finally {
     request.signal?.removeEventListener("abort", abortMerged);
   }

4. Request Cloning Behavior (lines 147-154)

   • The new cloneRequestWithSignal() creates a request from newUrl and request, then clones it again
   • This is simpler than the old approach but may have different behavior with request body consumption
   • Concern: If request.body has already been read or is a stream, this could fail
   • Recommendation: Add a comment explaining why double-cloning is safe here, or verify that the body hasn't been consumed

5. File Watcher State Management (lines 168-169)

   • Using undefined instead of null for lastMtime is fine, but the missingWarningShown flag is never reset if the file reappears
   • Observation: Actually, looking at line 190, missingWarningShown IS reset to false when the file exists. This is correct. ✅

Performance Considerations

✅ Good

• Reduced watch interval from 1000ms to 500ms improves responsiveness without significant overhead
• Extracted clearWatcher callback prevents duplicate code

⚠️ Minor Concerns

1. Interval Polling (line 214)
   • File watching via polling every 500ms is workable but not ideal for performance
   • The comments explain why this approach was chosen (cross-platform issues with fs watchers)
   • Acceptable tradeoff for development environment

Security Concerns

✅ No Major Issues

• No obvious security vulnerabilities introduced
• File watching is limited to development environment
• Proper error handling prevents information leakage

💡 Minor Observation

• The ROUTE_FILE path is constructed from process.cwd(), which is generally safe but assumes the cwd is the project root
• This is reasonable for Next.js projects

Test Coverage

⚠️ Missing Tests

• No test files were modified or added in this PR
• Recommendation: Consider adding tests for:
  • Request cloning behavior with different request properties
  • File watcher abort behavior
  • Error fallback logic (lines 119-125)
  • Stream wrapping with waitForStreamFinish

Suggested Test Cases

1. Test that DELETE method is properly handled
2. Test request abort signal propagation
3. Test file watcher cleanup on various termination paths
4. Test fallback handler when main handler throws

Additional Observations

dev-env.sh Script

• Good: Provides clear documentation for development environment variables
• Good: Reduced timeouts make local development iteration faster
• Question: Should this be documented somewhere (like in CLAUDE.md or a main README)?
• Minor: Consider adding a note about sourcing this file in development setup documentation

Consistency with CLAUDE.md Guidelines

• ✅ Uses structured logging with tracing patterns
• ✅ Log messages are lowercase
• ✅ Comments are complete sentences
• ✅ No unnecessary documentation of deltas

Overall Assessment

Rating: Good 👍

This is a solid refactoring that improves code quality, error handling, and developer experience. The main concerns are around request signal handling and potential memory leaks from event listeners not being cleaned up. These are not critical bugs but should be addressed for production robustness.

Recommended Actions Before Merge

1. High Priority: Add null check for request.signal before accessing it
2. Medium Priority: Review event listener cleanup to prevent memory leaks
3. Medium Priority: Add comment explaining request cloning behavior
4. Low Priority: Add tests for new error handling paths
5. Low Priority: Document the dev-env.sh script in project documentation

Questions for Author

1. Is the double event listener registration for request abort intentional? (lines 98 and 104-107)
2. Have you tested with requests that have already consumed bodies?
3. Should dev-env.sh be mentioned in setup documentation?

---

Review generated by Claude Code

[graphite-app]

The fallback fetch call does not include the mergedController.signal. This means the fallback request cannot be aborted via the merged signal, causing inconsistent abort behavior between the primary and fallback code paths. The watchIntervalId is cleared but the fallback fetch may continue indefinitely.

return await fetch(new Request(newUrl, request), {
    signal: mergedController.signal,
});

Suggested change

	return await fetch(new Request(newUrl, request));
	return await fetch(new Request(newUrl, request), {
	signal: mergedController.signal,
	});

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Import statements need to be sorted according to Biome linter rules. Built-in modules like 'path' should come first, followed by external packages, then local imports. Reorder to: import { existsSync, join, statSync } from 'path'; import { Registry } from 'rivetkit/registry'; import { stringifyError } from 'rivetkit/utils'; import { logger } from './log';

Spotted by Graphite Agent (based on CI logs)



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Merge activity

• Jan 22, 6:58 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Jan 22, 6:59 AM UTC: CI is running for this pull request on a draft pull request (#4007) due to your merge queue CI optimization settings.
• Jan 22, 7:00 AM UTC: Merged by the Graphite merge queue via draft PR: #4007.