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

chore(core): log correct endpoint from registry#1341

Closed
NathanFlurry wants to merge 1 commit intomainfrom
10-03-chore_core_log_correct_endpoint_from_registry
Closed

chore(core): log correct endpoint from registry#1341
NathanFlurry wants to merge 1 commit intomainfrom
10-03-chore_core_log_correct_endpoint_from_registry

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 3, 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 3, 2025 10:18pm

Copy link
Member Author


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.

@NathanFlurry NathanFlurry force-pushed the 10-03-chore_core_log_correct_endpoint_from_registry branch from e0adbeb to 64e3e3d Compare October 3, 2025 22:16
@claude
Copy link

claude bot commented Oct 3, 2025

Claude finished @NathanFlurry's task —— View job


Pull Request Review

Reviewing PR: "chore(core): log correct endpoint from registry"

Todo List

  • Read and analyze the changed file
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Determine test coverage needs
  • Provide comprehensive feedback

Analysis

Change Summary: The PR fixes a hardcoded endpoint URL in the console output. Instead of always printing http://127.0.0.1:6420, it now correctly displays the actual config.endpoint value.

Files Changed: packages/rivetkit/src/registry/mod.ts:108

- console.log(`  - Endpoint:     http://127.0.0.1:6420`);
+ console.log(`  - Endpoint:     ${config.endpoint}`);

Review Feedback

Code Quality & Best Practices

  • Good: The change follows the existing pattern used in the start() method (lines 105-109), which already has proper conditional logic for displaying endpoints
  • Good: Maintains consistent formatting and spacing with surrounding code
  • Good: Uses the same approach as the non-serverless version for consistency

Potential Bugs/Issues

  • Fixed Bug: This change fixes a significant bug where the console output always showed http://127.0.0.1:6420 regardless of the actual endpoint configuration
  • Potential Issue: The config.endpoint can be null (from packages/rivetkit/src/client/config.ts:27-30). If config.endpoint is null, this will print "null" in the console output
  • Recommendation: Consider adding a null check similar to how it's handled in the start() method

Performance Considerations

  • No Impact: This is a simple string interpolation change with no performance implications
  • Good: No unnecessary computations or object allocations introduced

Security Concerns

  • No Issues: The endpoint is configuration data that's meant to be displayed, so no security concerns with logging it
  • Good: No sensitive information exposure since endpoints are typically public-facing addresses

⚠️ Consistency Issue

The start() method has more robust endpoint display logic:

if (config.disableServer) {
    console.log(`  - Endpoint:     (default server disabled)`);
} else {
    console.log(`  - Endpoint:     ${config.endpoint}`);
}

However, the startServerless() method doesn't check config.disableServer and always displays the endpoint. This inconsistency should be addressed.

Test Coverage

  • Missing: No apparent test coverage for console output formatting
  • Low Priority: Since this is display logic, unit tests may not be critical, but integration tests ensuring correct endpoint display would be valuable

Recommendations

  1. Add null safety (Critical):

    console.log(`  - Endpoint:     ${config.endpoint || '(not configured)'}`);
  2. Consider consistency with the start() method:

    if (config.disableServer) {
        console.log(`  - Endpoint:     (default server disabled)`);
    } else {
        console.log(`  - Endpoint:     ${config.endpoint || '(not configured)'}`);
    }

Summary

This is a good bug fix that resolves incorrect endpoint logging. The change is simple and correct, but could benefit from null safety and consistency improvements with the existing start() method implementation.


@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 3, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: 64e3e3d

@graphite-app
Copy link

graphite-app bot commented Oct 4, 2025

Merge activity

  • Oct 4, 2:18 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 4, 2:19 AM UTC: CI is running for this pull request on a draft pull request (#1342) due to your merge queue CI optimization settings.
  • Oct 4, 2:20 AM UTC: Merged by the Graphite merge queue via draft PR: #1342.

@graphite-app graphite-app bot closed this Oct 4, 2025
@graphite-app graphite-app bot deleted the 10-03-chore_core_log_correct_endpoint_from_registry branch October 4, 2025 02:20
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