Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Nov 3, 2025

No description provided.

@vercel
Copy link

vercel bot commented Nov 3, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-site Ready Ready Preview Comment Nov 3, 2025 7:17pm
rivetkit-serverless Error Error Nov 3, 2025 7:17pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 3, 2025 7:17pm
rivet-inspector Ignored Ignored Preview Nov 3, 2025 7:17pm

Copy link
Contributor Author

jog1t commented Nov 3, 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 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.

@claude
Copy link

claude bot commented Nov 3, 2025

PR Review: Use Correct Port in Examples

Summary

This PR updates port references in example files from 8080 to 6420 and adds a getActorHibernationConfig implementation. Overall, the changes are mostly correct but there are some critical inconsistencies that need to be addressed.


Critical Issues

1. Port Mismatch in Server Configuration Files

Several server files are listening on port 8080 but logging messages claim they are listening on port 6420:

  • examples/elysia/src/server.ts:17-19
  • examples/express/src/server.ts:20-22
  • examples/hono-bun/src/backend/server.ts:45-51

Impact: Servers actually listen on port 8080, but log messages claim port 6420, and client code now tries to connect to port 6420. Result: Connections will fail.

Fix Required: Change .listen(8080) to .listen(6420) in all these files.


Code Quality Issues

2. Incomplete Function Signature

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:110-115

The new getActorHibernationConfig function signature does not match the expected interface from runner/src/mod.ts which expects 3 parameters: (actorId: string, requestId: ArrayBuffer, request: Request)

Issues:

  • Missing the request parameter
  • Parameters are unused (actorId, requestId are never referenced)
  • lastMsgIndex is set to 0 but the type allows number or undefined

Recommendations:

  1. Add the missing request parameter to match the interface
  2. Add TypeScript types for clarity
  3. Consider if lastMsgIndex should be undefined instead of 0 when hibernation is disabled
  4. Add a comment explaining why hibernation is always disabled

Testing Concerns

3. Missing Test Coverage

  • No test files included in this PR
  • Port change affects 34 files across multiple examples
  • The new getActorHibernationConfig function has no tests
  • Risk of breaking existing examples if port mismatches are not caught

Recommendation: Add integration tests to verify examples work with the new port


Positive Aspects

  1. Consistent client-side updates: All React/frontend client code correctly updated to use port 6420
  2. Documentation alignment: README files properly updated
  3. Scope appropriateness: The PR focuses on a specific, well-defined task

Security & Performance

No security issues or performance concerns identified.


Recommendations Summary

Must Fix (Blocking):

  1. Update all .listen(8080) and Bun.serve port: 8080 to use port 6420
  2. Add the missing request parameter to getActorHibernationConfig

Should Fix (Non-blocking):

  1. Add proper TypeScript types to getActorHibernationConfig
  2. Consider changing lastMsgIndex: 0 to lastMsgIndex: undefined
  3. Add a comment explaining the hibernation config

Nice to Have:

  1. Add test coverage for the port changes

Verdict

⚠️ Changes Requested - The port mismatches must be fixed before merging to prevent breaking the examples.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 3, 2025

Graphite Automations

"Test" took an action on this PR • (11/03/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

This was referenced Nov 3, 2025
@jog1t jog1t merged commit 68f2ff8 into graphite-base/3306 Nov 6, 2025
7 of 11 checks passed
@jog1t jog1t deleted the 10-31-docs_use_correct_port_in_examples branch November 6, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants