Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 31, 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 Nov 4, 2025 8:26pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 8:26pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 8:26pm
rivet-site Ignored Ignored Preview Nov 4, 2025 8:26pm

Copy link
Member Author

NathanFlurry commented Oct 31, 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 Oct 31, 2025

PR Review: Add version to error logs

Summary

This PR improves error logging by adding the RivetKit version to error logs through a centralized EXTRA_ERROR_LOG constant. This is a good practice that will help with debugging and support.

Positive Aspects

DRY Principle: Excellent refactoring! Consolidating the repeated issues and support fields into a single EXTRA_ERROR_LOG constant reduces duplication across the codebase.

Better Observability: Including the version in error logs is a best practice that will make it easier to correlate bug reports with specific releases.

Consistent Implementation: The change is applied consistently across all error logging locations in both common/utils.ts and engine-process/mod.ts.

Clean Code: The spread operator usage (...EXTRA_ERROR_LOG) is idiomatic and readable.

Minor Issues

1. Unused Import (Low Priority)

In common/utils.ts:4, VERSION is imported but never used directly in the file:

import { EXTRA_ERROR_LOG, getEnvUniversal, VERSION } from "@/utils";

Since VERSION is only used via EXTRA_ERROR_LOG, you can remove it from the import:

import { EXTRA_ERROR_LOG, getEnvUniversal } from "@/utils";

2. Consider Naming Convention

The constant name EXTRA_ERROR_LOG could be more descriptive. Consider alternatives like:

  • ERROR_LOG_METADATA
  • ERROR_CONTEXT_FIELDS
  • STANDARD_ERROR_FIELDS

This is subjective, but these names might better convey that this object contains standard metadata fields included in all error logs.

Code Quality Assessment

Structure: ⭐⭐⭐⭐⭐ Clean refactoring with proper centralization
Maintainability: ⭐⭐⭐⭐⭐ Much easier to maintain than duplicated strings
Testing: ⭐⭐⭐ No tests added, but the change is straightforward

Recommendations

  1. Remove the unused VERSION import from common/utils.ts
  2. (Optional) Consider a more descriptive constant name
  3. No additional tests needed for this change

Security & Performance

✅ No security concerns
✅ No performance impact (if anything, slightly better due to object reuse)

Conclusion

This is a solid improvement to the codebase. The refactoring makes the code more maintainable and adds valuable debugging information. The only actionable item is removing the unused import. Approved with minor suggestions!

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 31, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 9798eaf

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from 3a63654 to 1a25cce Compare October 31, 2025 23:23
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from febb029 to 248a1ce Compare October 31, 2025 23:23
@claude
Copy link

claude bot commented Oct 31, 2025

Pull Request Review

Summary

This PR refactors error logging to include version information alongside existing support links. The changes consolidate duplicate logging metadata into a reusable EXTRA_ERROR_LOG constant.

Positives

Good Code Organization

  • DRY Principle: The refactoring eliminates duplicate code by consolidating the error logging metadata (issues, support, version) into a single constant EXTRA_ERROR_LOG
  • Maintainability: Future updates to error logging metadata only require changes in one location
  • Consistency: Ensures all error logs include the same metadata fields across the codebase

Version Tracking

  • Debugging Value: Including the version in error logs will significantly help with debugging user-reported issues, especially when supporting multiple versions in production

Observations

Implementation Details
The changes affect 3 files:

  1. src/utils.ts - Defines the new EXTRA_ERROR_LOG constant
  2. src/common/utils.ts - Refactors 4 error logging sites to use the constant
  3. src/engine-process/mod.ts - Refactors 2 error logging sites to use the constant

Import Consistency

  • Imports are added at the top of files (follows CLAUDE.md guidelines)
  • Existing import from @/utils in common/utils.ts is extended to include new exports

Suggestions

Minor: Consider TypeScript Type Safety
The EXTRA_ERROR_LOG constant could benefit from an explicit type annotation to ensure type safety and better IDE support. This would make the constant readonly to prevent accidental mutations and provide explicit type information for better autocomplete.

Optional: Test Coverage
While this is a low-risk refactoring, consider adding a simple unit test to verify the EXTRA_ERROR_LOG object has all expected keys and that the version field matches the package.json version.

Security & Performance

  • No security concerns - Changes only affect logging metadata
  • No performance impact - Spread operator usage is negligible in error paths
  • No breaking changes - Internal refactoring with no API changes

Conclusion

This is a solid refactoring that improves code quality and adds useful debugging information. The changes are low-risk and follow good software engineering practices. The optional suggestions above would add polish but are not blockers.

Recommendation: Approve with optional improvements

@claude
Copy link

claude bot commented Oct 31, 2025

Code Review - PR #3303: chore(rivetkit): log version with error logs

Summary

This PR refactors error logging to centralize the common error metadata (issues URL, support URL, and version) into a single constant EXTRA_ERROR_LOG. This is a good improvement that reduces duplication and ensures consistent version logging across all error scenarios.

✅ Positives

  1. DRY Principle: Excellent refactoring that eliminates repeated literals across multiple error logging sites
  2. Version Tracking: Adding version information to error logs is valuable for debugging user-reported issues
  3. Consistency: Ensures all error logs include the same support/issues links and version info
  4. Clean Implementation: The EXTRA_ERROR_LOG constant is simple and well-placed in utils.ts

🔍 Code Quality & Best Practices

Good:

  • The refactoring is clean and straightforward
  • Proper use of spread operator to merge error log metadata
  • All modified locations correctly import and use the new constant

Minor Considerations:

  • The constant name EXTRA_ERROR_LOG is descriptive and clear
  • VERSION is properly sourced from package.json (line 6-8 in utils.ts)

🐛 Potential Issues

None identified. The changes are low-risk:

  • Simple object spread operations
  • No logic changes, purely refactoring
  • TypeScript will catch any import/type issues at compile time

⚡ Performance Considerations

No concerns:

  • The EXTRA_ERROR_LOG object is created once at module initialization
  • Object spread is negligible overhead for error logging scenarios
  • No additional allocations compared to previous inline object literals

🔒 Security Concerns

None. The changes don't introduce any security issues:

  • URLs are static strings pointing to legitimate Rivet resources
  • Version string is pulled from package.json (standard practice)
  • No user input or sensitive data is exposed

🧪 Test Coverage

Current state:

  • This is a logging refactor with no behavioral changes
  • Existing tests should continue to pass
  • No new test coverage appears necessary

Recommendation:

  • If there are integration/E2E tests that check error log output format, verify they still pass
  • Consider spot-checking a few error scenarios manually to confirm version appears in logs

📝 Additional Notes

  1. Consistency with Rust codebase: This is a TypeScript-only change in the rivetkit package, so no alignment needed with Rust error handling patterns mentioned in CLAUDE.md

  2. Future improvement opportunity: If version logging becomes important in other contexts (non-error logs), consider whether VERSION export should be more prominently documented or if EXTRA_ERROR_LOG should be renamed to something more specific like ERROR_CONTEXT_METADATA

✨ Recommendation

LGTM - Approve and merge. This is a clean refactoring that improves maintainability and adds useful debugging information to error logs without introducing any risks.

@claude
Copy link

claude bot commented Nov 1, 2025

PR Review: chore(rivetkit): log version with error logs

Overview

This PR refactors error logging to include version information by centralizing error log metadata into a shared constant EXTRA_ERROR_LOG. This is a good improvement for debugging and issue reporting.

Code Quality ✅

Strengths:

  • Good use of DRY principle - consolidating repeated error log fields into a single constant
  • Clean refactoring across multiple files with consistent usage
  • Version information will be helpful for debugging user-reported issues
  • The change is focused and doesn't introduce unnecessary complexity

Observations:

  • The EXTRA_ERROR_LOG constant is well-placed in src/utils.ts alongside the VERSION constant it uses
  • All usages are consistent with the spread operator pattern

Potential Issues ⚠️

1. Missing Import in common/utils.ts
The file src/common/utils.ts:4 imports VERSION from @/utils, but VERSION is not actually used in this file - only EXTRA_ERROR_LOG is used. This import can be cleaned up:

// Current:
import { EXTRA_ERROR_LOG, getEnvUniversal, VERSION } from "@/utils";

// Should be:
import { EXTRA_ERROR_LOG, getEnvUniversal } from "@/utils";

2. Consistency Check
I notice the diff shows changes in error logging, but I'd recommend grepping the codebase to ensure ALL instances of the hardcoded URLs have been replaced:

rg "https://github.com/rivet-dev/rivetkit/issues" --type ts
rg "https://rivet.dev/discord" --type ts

If there are any remaining hardcoded instances, they should also be updated to use EXTRA_ERROR_LOG.

Performance Considerations ✅

  • The change is performance-neutral - object spread at logging time is negligible
  • The EXTRA_ERROR_LOG constant is created once at module load time

Security Considerations ✅

  • No security concerns
  • The version information being logged is already public information from package.json

Test Coverage 💭

Observations:

  • This is a logging change that doesn't alter functional behavior
  • No new tests are strictly required, but consider:
    • Verifying that error logs contain the version field in existing test suites
    • The test file rivetkit-typescript/packages/rivetkit/tests/engine-process.test.ts might have assertions that could be updated to verify version is logged

Recommendation: While not critical, if there are integration tests that capture log output, it would be good to verify the new fields are present.

Suggestions

  1. Remove unused import: Clean up the VERSION import in src/common/utils.ts:4
  2. Verify completeness: Run a grep to ensure all hardcoded URLs have been replaced
  3. Consider adding a comment: Add a brief JSDoc comment to EXTRA_ERROR_LOG explaining its purpose:
    /**
     * Standard fields to include in error logs for debugging and user support.
     * Includes links to issue tracker, support channel, and current library version.
     */
    export const EXTRA_ERROR_LOG = {
      issues: "https://github.com/rivet-dev/rivetkit/issues",
      support: "https://rivet.dev/discord",
      version: VERSION,
    };

Conclusion

This is a solid refactoring that improves maintainability and debugging capability. The changes are minimal and focused. After addressing the unused import and verifying all instances are updated, this is ready to merge.

Status: ✅ Approved with minor suggestions


Generated with Claude Code

@claude
Copy link

claude bot commented Nov 1, 2025

Code Review: chore(rivetkit): log version with error logs

Summary

This PR refactors error logging to include version information by centralizing the error metadata (issues, support, version) into a reusable EXTRA_ERROR_LOG constant. This is a good improvement for debugging and support purposes.

Positive Observations ✅

  1. DRY Principle: Excellent refactoring that eliminates code duplication by consolidating the repeated error metadata fields into a single constant.

  2. Enhanced Debugging: Including the version in error logs will significantly help with debugging and support, making it easier to correlate errors with specific releases.

  3. Consistent Implementation: The change is applied consistently across all error logging locations in the codebase.

  4. Clean Import Usage: Proper use of named imports for the new constant.

Code Quality

File: rivetkit-typescript/packages/rivetkit/src/utils.ts:267-271

  • The EXTRA_ERROR_LOG constant is well-structured and references the already-exported VERSION constant
  • Good placement at the end of the utils file
  • Clear, descriptive naming

Files: Error handlers

  • All replacements maintain the same spread operator pattern (...EXTRA_ERROR_LOG)
  • No behavioral changes, purely refactoring

Potential Considerations

  1. Const vs Object Literal: The EXTRA_ERROR_LOG is a plain object, which means it could technically be mutated. Consider using as const for stronger type safety:

    export const EXTRA_ERROR_LOG = {
        issues: "https://github.com/rivet-dev/rivetkit/issues",
        support: "https://rivet.dev/discord",
        version: VERSION,
    } as const;
  2. Type Safety: Consider defining an explicit type for error log metadata to ensure consistency:

    export interface ErrorLogMetadata {
        issues: string;
        support: string;
        version: string;
    }
    
    export const EXTRA_ERROR_LOG: ErrorLogMetadata = { ... };
  3. Testing: There are no test additions for this change. While this is primarily a refactoring, consider:

    • Verifying the version is correctly included in error logs
    • Ensuring the VERSION import works correctly in the build output

Security & Performance

  • ✅ No security concerns
  • ✅ No performance impact (if anything, slight improvement by avoiding repeated object creation)
  • ✅ No new dependencies

Recommendations

Minor (Non-blocking):

  1. Add as const assertion for immutability
  2. Consider adding a simple test to verify version is included in error logs
  3. Consider defining an explicit type for the error metadata

Conclusion

This is a clean, well-executed refactoring that improves maintainability and debugging capabilities. The changes follow good software engineering practices and will make it easier to support users by including version information in error logs.

Status: Approved ✓

Great work on this improvement! The changes are straightforward, consistent, and add valuable debugging information.

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from 74dfc47 to df50a4b Compare November 2, 2025 16:05
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from 80b8e25 to c6e7414 Compare November 2, 2025 16:05
@claude
Copy link

claude bot commented Nov 2, 2025

PR Review: chore(rivetkit): log version with error logs

Summary

This PR improves error logging by consolidating repeated error metadata (issues URL, support URL, and version) into a reusable constant EXTRA_ERROR_LOG. This is a good refactoring that reduces duplication and ensures version information is consistently included in error logs.

Code Quality ✅

Strengths:

  • DRY Principle: Excellent consolidation of repeated code. The PR replaces 5 instances of duplicate metadata with a single constant.
  • Consistency: Ensures all error logs include the same support information and version.
  • Maintainability: Future changes to error metadata only need to be made in one place.
  • Clean implementation: The spread operator usage is idiomatic and clean.

Minor Observations:

  • The constant is properly exported and imported where needed.
  • The naming EXTRA_ERROR_LOG is descriptive and follows TypeScript conventions.

Potential Issues 🔍

1. VERSION dependency:
The EXTRA_ERROR_LOG constant references VERSION which is defined at module initialization:

export const VERSION = pkgJson.version;
export const EXTRA_ERROR_LOG = {
    issues: "https://github.com/rivet-dev/rivetkit/issues",
    support: "https://rivet.dev/discord",
    version: VERSION,
};

This is fine for a static constant, but worth noting that the version is read once at module load time. This is the expected behavior for package versions.

Performance Considerations ✅

Positive:

  • Using a constant object with spread operator is efficient.
  • No runtime overhead from string concatenation or object creation on each error.
  • The object is created once at module initialization.

Note: The spread operator creates a shallow copy each time it's used, which is negligible overhead for a small object like this.

Security Considerations ✅

No security concerns identified:

  • URLs are hardcoded constants (not user input)
  • Version comes from package.json (trusted source)
  • No injection or XSS risks

Test Coverage ⚠️

Missing:

  • No new tests added for the EXTRA_ERROR_LOG constant.
  • No tests verify that error logs include the version field.

Recommendation:
Consider adding a test to verify:

  1. The EXTRA_ERROR_LOG object contains expected fields
  2. Error logging functions properly spread the metadata
  3. The version field is populated correctly

Example test:

import { EXTRA_ERROR_LOG, VERSION } from "@/utils";

test("EXTRA_ERROR_LOG includes version", () => {
    expect(EXTRA_ERROR_LOG.version).toBe(VERSION);
    expect(EXTRA_ERROR_LOG.issues).toBe("https://github.com/rivet-dev/rivetkit/issues");
    expect(EXTRA_ERROR_LOG.support).toBe("https://rivet.dev/discord");
});

Additional Suggestions 💡

  1. Documentation: Consider adding a JSDoc comment to EXTRA_ERROR_LOG explaining its purpose:
/**
 * Standard metadata included in error logs for debugging and support.
 * Contains issue tracker URL, support channel, and package version.
 */
export const EXTRA_ERROR_LOG = {
    issues: "https://github.com/rivet-dev/rivetkit/issues",
    support: "https://rivet.dev/discord",
    version: VERSION,
};
  1. Type Safety: Consider adding a type definition to make the structure explicit:
export interface ErrorLogMetadata {
    issues: string;
    support: string;
    version: string;
}

export const EXTRA_ERROR_LOG: ErrorLogMetadata = {
    // ...
};

Conclusion ✅

This is a solid refactoring that improves code maintainability and adds valuable debugging information (version) to error logs. The implementation is clean and follows good practices.

Recommendation: Approve with minor suggestions

The lack of tests is not blocking for this simple refactoring, but adding them would improve confidence in future changes.

@claude
Copy link

claude bot commented Nov 2, 2025

PR Review: chore(rivetkit): log version with error logs

Summary

This PR introduces a centralized constant EXTRA_ERROR_LOG that includes version information alongside issue and support URLs in error logs. This is a good improvement for debugging and issue reporting.

Code Quality ✅

Strengths:

  • Good use of DRY principle by creating a reusable constant instead of repeating the same fields across multiple log statements
  • Consistent implementation across all affected files
  • Clear and focused change that does exactly what the title says

Code locations reviewed:

  • rivetkit-typescript/packages/rivetkit/src/utils.ts:267-271 - New constant definition
  • rivetkit-typescript/packages/rivetkit/src/common/utils.ts - 4 replacements
  • rivetkit-typescript/packages/rivetkit/src/engine-process/mod.ts - 2 replacements

Potential Issues ⚠️

  1. Version value source: The VERSION constant is imported from @/utils and defined as pkgJson.version (line 8 of utils.ts). This is reading from package.json at build/runtime. While this works, be aware that:

    • If this is used in a bundled context, the version will be frozen at bundle time
    • Ensure the package.json import with type assertion is properly supported in all target runtimes (Node, Deno, Bun, Cloudflare Workers as mentioned in the package description)
  2. Missing import in utils.ts: In rivetkit-typescript/packages/rivetkit/src/utils.ts:267-271, the EXTRA_ERROR_LOG constant references VERSION which is defined on line 8, so this is fine. However, ensure that any future consumers of this constant also have access to VERSION.

Best Practices ✅

  • Follows the project's structured logging patterns (lowercase messages, structured fields)
  • Naming convention is appropriate (EXTRA_ERROR_LOG is descriptive and follows SCREAMING_SNAKE_CASE for constants)
  • The spread operator usage (...EXTRA_ERROR_LOG) is clean and idiomatic

Test Coverage 📋

Observation: No tests were added or modified in this PR. While this is a simple refactoring, consider:

  • Adding a test to verify that error logs include the version field
  • Verifying that the version is correctly read from package.json in different runtime environments

The package has a test suite (vitest) and several existing tests in the tests/ directory. Consider adding a simple test like:

import { EXTRA_ERROR_LOG, VERSION } from '@/utils';

test('EXTRA_ERROR_LOG includes version', () => {
  expect(EXTRA_ERROR_LOG.version).toBe(VERSION);
  expect(EXTRA_ERROR_LOG.issues).toBe('https://github.com/rivet-dev/rivetkit/issues');
  expect(EXTRA_ERROR_LOG.support).toBe('https://rivet.dev/discord');
});

Security Concerns ✅

No security issues identified. The change only adds version information to logs, which is helpful for debugging and does not expose sensitive information.

Performance Considerations ✅

  • Minimal performance impact - the constant is created once at module load time
  • Spreading the object in log calls is efficient
  • No runtime overhead compared to the previous inline approach

Recommendations

  1. Add a test to verify the EXTRA_ERROR_LOG constant structure and that version is correctly populated
  2. Verify runtime compatibility: Ensure the package.json import works correctly across all supported runtimes (Node, Deno, Bun, Cloudflare Workers)
  3. Consider documenting: If this pattern should be used elsewhere in the codebase for consistency, consider adding a comment or guideline

Conclusion

This is a solid, focused change that improves maintainability and debugging capability. The main suggestion is to add test coverage to prevent regressions. Overall: LGTM with minor suggestions


Review conducted following the repository's CLAUDE.md guidelines

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from df50a4b to 62dbc05 Compare November 2, 2025 16:20
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from c6e7414 to 41fc7d5 Compare November 2, 2025 16:20
@NathanFlurry NathanFlurry changed the base branch from 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning to graphite-base/3303 November 4, 2025 17:45
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3303 to 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning November 4, 2025 18:38
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from 2633432 to a1e6014 Compare November 4, 2025 18:59
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from d0fce8f to 718535f Compare November 4, 2025 18:59
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): log version with error logs

Overview

This PR consolidates error logging metadata by introducing a shared EXTRA_ERROR_LOG constant that includes the rivetkit version alongside existing support links. This is a good improvement for debugging and user support.

✅ Code Quality

Strengths:

  • DRY principle: Excellent refactoring that eliminates duplicated error metadata across multiple files
  • Clear naming: EXTRA_ERROR_LOG clearly conveys its purpose
  • Consistent usage: Applied uniformly across all error logging call sites
  • Maintainability: Centralizing this metadata makes future updates much easier (e.g., if support links change)

📋 Changes Summary

The PR makes the following changes across 3 files:

  1. utils.ts - Defines EXTRA_ERROR_LOG constant with issues, support, and version fields
  2. common/utils.ts - Replaces 4 instances of hardcoded metadata in deconstructError()
  3. engine-process/mod.ts - Replaces 2 instances in process lifecycle error handlers

🎯 Best Practices

Follows conventions:

  • ✅ Imports are properly organized at the top of files (per CLAUDE.md guidelines)
  • ✅ Uses structured logging pattern with spread operator
  • ✅ Leverages existing VERSION constant from package.json

🔍 Potential Considerations

  1. Export clarity - Consider whether EXTRA_ERROR_LOG should be explicitly exported in utils.ts, though the current named export pattern works fine.

  2. Type safety - While not strictly necessary for this simple object, you could add a type definition for better IDE support:

    export interface ExtraErrorLog {
      issues: string;
      support: string;
      version: string;
    }
    
    export const EXTRA_ERROR_LOG: ExtraErrorLog = {
      // ...
    };
  3. Documentation - A brief JSDoc comment explaining the purpose would help future maintainers:

    /**
     * Standard metadata included with error logs to help users report issues.
     * Includes support links and the current rivetkit version.
     */
    export const EXTRA_ERROR_LOG = {
      // ...
    };

🐛 Bugs/Issues

None identified - The implementation is straightforward and correct.

🔒 Security

No concerns - Only adds version metadata to logs, which is helpful for debugging and doesn't expose sensitive information.

⚡ Performance

No impact - The constant is created once at module load time and reused via spread operator. Negligible overhead.

🧪 Testing

The changes are primarily refactoring existing behavior. Consider:

  • ✅ Ensuring existing tests still pass (no new test coverage needed for this refactor)
  • 💡 If there are integration tests that validate error log structure, verify they handle the new version field

📝 Summary

This is a clean, well-executed refactoring that improves maintainability and adds useful debugging information. The version field will be particularly helpful when troubleshooting user-reported issues.

Recommendation: Approve ✅

The minor suggestions above (types, docs) are optional nice-to-haves and shouldn't block merging.

@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

Summary

This PR refactors error logging to include version information by introducing a centralized EXTRA_ERROR_LOG constant. The change consolidates repeated logging metadata (GitHub issues link, Discord support link, and version) into a single reusable object.

Positive Aspects ✓

  1. DRY Principle: Excellent refactoring that eliminates code duplication across 4 call sites
  2. Maintainability: Changes to error log metadata now only need to be made in one place
  3. Version Tracking: Adding version information to error logs is valuable for debugging and support
  4. Clean Implementation: The spread operator usage is idiomatic and maintains existing log structure

Code Quality

Good:

  • The refactoring maintains existing functionality while improving code organization
  • Import statements are properly updated across all affected files
  • The constant is appropriately exported and named

Suggestions for Improvement

1. TypeScript Type Safety

Consider adding an explicit type annotation to EXTRA_ERROR_LOG to ensure consistency:

export const EXTRA_ERROR_LOG: {
  readonly issues: string;
  readonly support: string;
  readonly version: string;
} = {
  issues: "https://github.com/rivet-dev/rivetkit/issues",
  support: "https://rivet.dev/discord",
  version: VERSION,
} as const;

This would:

  • Provide better IDE autocomplete for consumers
  • Prevent accidental mutations
  • Make the contract explicit

2. Documentation

Add a JSDoc comment to explain the purpose:

/**
 * Standard metadata included in all error logs for support and debugging.
 * Includes links to report issues and get support, plus the current library version.
 */
export const EXTRA_ERROR_LOG = { /* ... */ };

3. Test Coverage Consideration

While this is primarily a refactoring, consider adding a test to verify that error logs include the expected fields. This could prevent regression if the logging structure changes in the future.

Potential Concerns

None identified - This is a straightforward, low-risk refactoring.

Security & Performance

  • Security: No concerns. The change only affects logging, not authentication or data handling
  • Performance: Negligible impact. Object spread is efficient and these code paths only execute during error conditions

Final Assessment

This is a clean, well-executed refactoring that improves code maintainability. The suggestions above are optional enhancements rather than blocking issues.

Recommendation: ✅ Approve with optional suggestions


Note: Review was automated but checked for code quality, best practices, potential bugs, performance, security, and test coverage per repository standards.

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from 718535f to 11ff7b7 Compare November 4, 2025 19:24
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from a1e6014 to 0acf499 Compare November 4, 2025 19:24
@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 4 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): log version with error logs

Summary

This PR consolidates repeated error log metadata (GitHub issues and Discord support links) plus the new version information into a single reusable EXTRA_ERROR_LOG constant. This is a solid DRY (Don't Repeat Yourself) refactoring that adds valuable debugging information.

✅ Positive Aspects

  1. DRY Principle: Excellent consolidation of repeated URL strings into a single constant, reducing code duplication and making future updates easier.

  2. Improved Debugging: Adding the version field to error logs is very helpful for troubleshooting user-reported issues - you'll immediately know which version of rivetkit they're running.

  3. Maintainability: If the support links ever need to change, there's now a single source of truth.

  4. Consistent Implementation: The refactoring is applied consistently across all error logging sites in the codebase.

💡 Suggestions

  1. TypeScript Type Definition: Consider adding a type definition for EXTRA_ERROR_LOG to make it more explicit and provide better IDE support:
export const EXTRA_ERROR_LOG = {
	issues: "https://github.com/rivet-dev/rivetkit/issues",
	support: "https://rivet.dev/discord",
	version: VERSION,
} as const;

The as const assertion would make the object readonly and provide more precise typing.

  1. Documentation: Consider adding a JSDoc comment to explain the purpose of EXTRA_ERROR_LOG:
/**
 * Additional metadata to include in error logs for user support and debugging.
 * Includes links to issue tracker and support channels, plus the current rivetkit version.
 */
export const EXTRA_ERROR_LOG = {
	// ...
};

✅ Security & Performance

  • No security concerns
  • No performance impact (purely structural refactoring)

✅ Testing

  • This change is low-risk and doesn't require additional tests
  • The existing error handling paths remain functionally identical
  • Consider manually verifying that error logs now include the version field in development

Conclusion

This is a clean, straightforward improvement. The PR achieves its goal effectively and follows good software engineering practices. LGTM! 🚀

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): log version with error logs

Summary

This PR refactors error logging to include version information by consolidating repeated URL and support information into a shared EXTRA_ERROR_LOG constant. This is a good improvement that reduces code duplication and makes version tracking easier for debugging.

Code Quality ✅

Strengths:

  • DRY principle applied well: The refactoring eliminates 4 instances of duplicated issues and support URLs by extracting them into a shared constant
  • Improved debuggability: Adding version information to error logs is valuable for troubleshooting user-reported issues
  • Clean implementation: The use of spread operator (...EXTRA_ERROR_LOG) makes the code more maintainable
  • Consistent naming: EXTRA_ERROR_LOG is descriptive and follows TypeScript naming conventions for constants

Potential Issues 🔍

Minor concerns:

  1. Unused import: In common/utils.ts:4, VERSION is imported but never directly used in the file (it's only used via EXTRA_ERROR_LOG). This is a minor cleanup opportunity.
// Line 4 - VERSION import appears unused
import { EXTRA_ERROR_LOG, getEnvUniversal, VERSION } from "@/utils";
  1. Export location: EXTRA_ERROR_LOG is defined at the bottom of utils.ts (line 267). Consider whether this should be closer to the top with other constants for better code organization, though this is largely a style preference.

Security ✅

No security concerns identified. The changes only affect logging and don't introduce any new attack vectors.

Performance ✅

No performance impact. The constant is defined once and referenced, which is optimal.

Test Coverage ⚠️

Observation: While the codebase has test files (including engine-process.test.ts which likely tests the modified engine-process/mod.ts), there don't appear to be tests specifically for the logging utility functions in common/utils.ts.

Recommendation: Consider adding tests to verify:

  • Error logs include all expected fields from EXTRA_ERROR_LOG
  • The version field is correctly populated
  • Different error types (public, internal, exposed internal) all include the extra logging fields

Example test case:

test('deconstructError includes version in logs', () => {
  const mockLogger = createMockLogger();
  deconstructError(new Error('test'), mockLogger, {});
  expect(mockLogger.lastLog).toHaveProperty('version');
  expect(mockLogger.lastLog.version).toMatch(/^\d+\.\d+\.\d+/);
});

Best Practices ✅

  • Follows TypeScript conventions
  • Maintains backwards compatibility
  • Improves maintainability
  • Good commit message following conventional commits format

Recommendations

  1. Remove unused import: Remove VERSION from the import statement in common/utils.ts:4 since it's not directly used
  2. Consider adding tests: Add unit tests to verify the error logging includes version information
  3. Documentation: Consider adding a JSDoc comment to EXTRA_ERROR_LOG explaining its purpose:
/**
 * Standard metadata included in all error logs for debugging and support.
 * Includes version information, issue tracker, and support channel links.
 */
export const EXTRA_ERROR_LOG = {
  issues: "https://github.com/rivet-dev/rivetkit/issues",
  support: "https://rivet.dev/discord",
  version: VERSION,
};

Verdict: ✅ Approve with Minor Suggestions

This is a solid improvement that enhances debuggability with minimal code changes. The suggestions above are minor and don't block approval. Great work on reducing code duplication!

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_close_code_reason_in_disconnect_warning branch from 11ff7b7 to f76b79c Compare November 4, 2025 20:25
@NathanFlurry NathanFlurry force-pushed the 10-31-chore_rivetkit_log_version_with_error_logs branch from 0acf499 to 9798eaf Compare November 4, 2025 20:25
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Add version to error logs

Summary

This PR consolidates repeated error log metadata (issues URL, support URL) and adds version information by creating a reusable EXTRA_ERROR_LOG constant. This is a good refactoring that improves maintainability and debugging capability.

✅ Strengths

  1. DRY Principle: Eliminates duplication of hardcoded URLs across multiple error logging sites
  2. Improved Debugging: Adding version information to error logs will significantly help with troubleshooting and issue triage
  3. Clean Implementation: The EXTRA_ERROR_LOG constant is well-placed in utils.ts and properly exported
  4. Consistent Application: All error logging call sites have been updated uniformly

📝 Code Quality

Good:

  • Clean use of object spread syntax (...EXTRA_ERROR_LOG)
  • Consistent import patterns
  • No behavioral changes, purely refactoring

Minor Observations:

  • The version is read from package.json at import time (line 6-8 in utils.ts), which is appropriate for this use case
  • All modified files maintain consistent formatting

🔍 Potential Considerations

  1. Version Information: The VERSION constant is sourced from package.json. This is correct for npm package versioning, but worth noting that:

    • It reflects the rivetkit package version, not the consuming application's version
    • This is appropriate since errors are being logged from rivetkit itself
  2. Testing: Consider verifying that error logs now include the version field in relevant test scenarios (though this is a low-risk change)

🐛 Bugs/Issues

None identified. The changes are straightforward and safe.

🔒 Security

No security concerns. This change only affects logging metadata.

⚡ Performance

Negligible impact. The EXTRA_ERROR_LOG object is created once at module load time and reused via spread operator.

✅ Test Coverage

This change doesn't introduce new logic that requires additional test coverage. The existing error handling tests should continue to work as expected.

📋 Recommendations

None required - This is a clean, well-executed refactoring.

Optional enhancements (not blockers):

  • Consider adding the version to structured logging in success cases as well, not just errors
  • Document the EXTRA_ERROR_LOG constant with a JSDoc comment explaining its purpose

Verdict

Approved - This is a solid improvement that enhances maintainability and debugging capability with minimal risk. Ready to merge.

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): log version with error logs

Summary

This PR consolidates repetitive error logging metadata (GitHub issues URL, Discord support URL) into a reusable constant EXTRA_ERROR_LOG and adds version information to all error logs. This improves debugging capabilities by including the RivetKit version in error reports.

Code Quality: ✅ Excellent

Strengths:

  • DRY Principle: Eliminates code duplication by centralizing the error metadata into a single constant
  • Maintainability: Future changes to error logging metadata only need to be made in one place
  • Debugging Enhancement: Adding version information to error logs is a valuable improvement for issue triage
  • Consistent Implementation: Applied uniformly across all error logging locations

Changes Review

rivetkit-typescript/packages/rivetkit/src/utils.ts

export const EXTRA_ERROR_LOG = {
	issues: "https://github.com/rivet-dev/rivetkit/issues",
	support: "https://rivet.dev/discord",
	version: VERSION,
};
  • ✅ Clean constant definition
  • ✅ Properly exports VERSION from package.json (line 8)
  • ✅ Version is already imported and used elsewhere in the file

rivetkit-typescript/packages/rivetkit/src/common/utils.ts

  • ✅ Replaces 4 instances of hardcoded URLs with ...EXTRA_ERROR_LOG
  • ✅ Proper spread operator usage maintains existing behavior
  • ✅ Covers all error logging paths: public errors, internal errors, and generic errors

rivetkit-typescript/packages/rivetkit/src/engine-process/mod.ts

  • ✅ Replaces 2 instances in engine process error handling
  • ✅ Covers both engine exit events and download failures

Potential Issues: None Found

I didn't identify any bugs, security concerns, or performance issues.

Suggestions for Consideration

Type Safety (Minor Enhancement):
Consider adding an explicit type for better IDE support:

export interface ErrorLogMetadata {
	issues: string;
	support: string;
	version: string;
}

export const EXTRA_ERROR_LOG: ErrorLogMetadata = {
	issues: "https://github.com/rivet-dev/rivetkit/issues",
	support: "https://rivet.dev/discord",
	version: VERSION,
};

However, this is optional and the current implementation is perfectly acceptable.

Test Coverage

The changes are to logging infrastructure and don't alter business logic, so existing tests should cover the functional behavior. Consider:

  • ✅ No new test coverage needed for this refactor
  • ℹ️ If there are integration tests that verify log output structure, ensure they pass with the new version field

Security: ✅ No Concerns

  • Version information in logs is appropriate for debugging
  • No sensitive information is being exposed
  • External URLs are legitimate (GitHub issues, Discord support)

Performance: ✅ No Impact

  • Object spread is negligible overhead
  • Constant is created once at module load time
  • No runtime performance implications

Overall Assessment: LGTM

This is a clean, well-executed refactoring that improves code maintainability and debugging capabilities. The change is low-risk and provides clear value.

Recommendation: Approve and merge 🚀

@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 10-31-chore_rivetkit_log_version_with_error_logs branch November 4, 2025 20: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