Skip to content

Conversation

@NathanFlurry
Copy link
Member

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)
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 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

Code Review

Summary

This PR applies Biome formatting to the TypeScript RivetKit SDK files. The changes are purely cosmetic formatting updates with no functional changes.

✅ Positives

  1. Consistent Formatting: The PR successfully applies consistent formatting across the codebase, which improves maintainability.

  2. Multi-line Parameter Formatting: Function signatures with multiple parameters are now properly formatted across multiple lines for better readability (e.g., closeCallback in websocket-tunnel-adapter.ts).

  3. Import Organization: The change in log.ts correctly uses node:util instead of util, which is the recommended Node.js import style for built-in modules.

  4. biome.json Update: Removing the blanket exclusion of !engine/sdks and replacing it with more specific exclusions (!engine/sdks/typescript/api-full and !engine/sdks/typescript/runner-protocol) is a good approach that allows formatting these SDK files while still excluding generated code.

📝 Observations

  1. No Functional Changes: All changes are formatting-only, which reduces the risk of introducing bugs.

  2. Consistent Style Application:

    • Proper spacing in function parameters
    • Consistent use of else statements
    • Proper line breaks for long expressions
    • Consistent use of empty arrow functions (() => {})

🔍 Code Quality

websocket-tunnel-adapter.ts:28-35

  • Good: Multi-line parameter formatting improves readability for complex function signatures

test-runner/src/index.ts:115

  • Good: The } else await formatting follows JavaScript best practices by keeping the else on the same line as the closing brace

test-runner/src/index.ts:226-238

  • Good: Breaking up the nested function calls and Buffer operations into multiple lines improves readability significantly

test-runner/src/log.ts:1

  • Good: Using node:util is the modern Node.js convention for importing built-in modules

⚠️ Considerations

  1. Testing: Since these are formatting-only changes, the existing tests should continue to pass without modification. Consider running the test suite to verify.

  2. CI/CD Integration: Ensure that Biome formatting checks are integrated into your CI pipeline to prevent unformatted code from being merged in the future.

  3. Team Communication: If this is the first time applying Biome to these files, ensure the team is aware that future contributions to these files should follow Biome formatting.

🎯 Recommendations

  1. Approve: This PR is safe to merge. The changes are formatting-only with no functional impact.

  2. Consider adding a pre-commit hook or CI check to automatically enforce Biome formatting on TypeScript files in the engine/sdks directory.

  3. The PR title chore(rivetkit): fmt follows conventional commit format, which is good.

Verdict

LGTM ✅ - This is a clean formatting PR with no issues. The changes improve code readability and consistency without introducing any functional changes or bugs.

@NathanFlurry NathanFlurry force-pushed the 11-03-chore_rivetkit_fmt branch from 94fcf04 to 8e871bc Compare November 4, 2025 00:36
@NathanFlurry NathanFlurry force-pushed the 11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs branch from cad68b3 to 6079f1f Compare November 4, 2025 00:36
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): fmt

Summary

This PR applies Biome formatting to TypeScript files in the engine/sdks/typescript directory by removing the blanket exclusion of !engine/sdks from biome.json. The changes are purely cosmetic formatting updates.

Code Quality ✅

Formatting Changes:

  • All changes are consistent with Biome's formatting rules
  • Multi-line function parameters are properly formatted for better readability
  • Import statements corrected to use Node.js protocol (node:util instead of util) per the project's linting rules (useNodejsImportProtocol: "error")
  • Consistent whitespace and line breaks

Files Modified:

  1. biome.json - Removed blanket SDK exclusion (line 9: !engine/sdks)
  2. websocket-tunnel-adapter.ts - Reformatted function signatures (constructor and _handleMessage)
  3. test-runner/src/index.ts - Multiple formatting improvements (whitespace, line breaks)
  4. test-runner/src/log.ts - Import statement updated to use Node.js protocol

Potential Issues 🟡

Scope Concerns:

  1. The biome.json change removes the entire !engine/sdks exclusion, but only a subset of SDK files are being formatted in this PR. This means:
    • Future commits will automatically format all files in engine/sdks (except the specific exclusions)
    • Other SDK files may have unformatted code that will cause linting failures
    • Consider running Biome on all SDK files to ensure consistency

Verification Needed:

# Check if other SDK files need formatting
biome check engine/sdks/typescript --write
  1. The PR only touches runner and test-runner packages. Are there other packages in engine/sdks/typescript/ that also need formatting?

Best Practices ✅

  • Follows the repository's formatting conventions
  • Changes are minimal and focused
  • No functional changes to code logic

Security Concerns ✅

No security issues identified. All changes are cosmetic formatting only.

Test Coverage 🟡

Current State:

  • Found one test file: runner/tests/lifecycle.test.ts
  • No tests were modified or added in this PR
  • This is expected for a formatting-only PR

Recommendation:
Since this is a formatting change, no new tests are needed. However, it would be good to:

  1. Run existing tests to ensure formatting didn't accidentally break anything
  2. Verify that the test-runner package has adequate test coverage for the WebSocket functionality

Performance Considerations ✅

No performance impact - formatting changes only.

Recommendations

  1. Complete the formatting: Run Biome on all files in engine/sdks/typescript/ to ensure no files are left partially formatted:

    biome check engine/sdks/typescript --write
  2. Verify tests pass: Run the test suite to ensure formatting didn't introduce any issues:

    # From the workspace root
    pnpm test
  3. Consider splitting changes: If other SDK packages need formatting, consider whether this should be:

    • One large PR formatting all SDK code
    • Multiple PRs per package for easier review
  4. Note about CLAUDE.md: The project's CLAUDE.md states "DO NOT RUN CARGO FMT AUTOMATICALLY" for Rust, but this is TypeScript formatting with Biome, which appears intentional and appropriate.

Overall Assessment

This is a clean, low-risk PR that improves code consistency. The main concern is ensuring all files in the engine/sdks directory are formatted consistently after removing the blanket exclusion. Once that's verified, this should be safe to merge.

Suggested next steps:

  1. Run Biome on entire engine/sdks/typescript directory
  2. Verify tests pass
  3. Merge

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review - PR #3331

Summary

This PR applies automated formatting (Biome) to the RivetKit TypeScript SDK packages. The changes are primarily formatting-related with no functional modifications.

✅ Positive Changes

  1. biome.json configuration: Removed the blanket exclusion of !engine/sdks and now uses more granular exclusions. This allows formatting to be applied to SDK packages that should be formatted (runner, test-runner).

  2. Import organization (log.ts): Changed from import { inspect } from "util" to import { inspect } from "node:util" following the Node.js protocol convention. This aligns with the project's Biome linter rule useNodejsImportProtocol: "error" in biome.json:34.

  3. Consistent formatting: All formatting changes (line breaks, indentation, spacing) follow standard Biome/Prettier conventions making the code more readable and maintainable.

📝 Observations

websocket-tunnel-adapter.ts changes:

  • Lines 28-36: Multi-line function parameter formatting for closeCallback
  • Lines 443-448: Multi-line parameter formatting for _handleMessage method
  • These improve readability for functions with multiple parameters

test-runner/index.ts changes:

  • Lines 107-108: Fixed else-if formatting
  • Lines 156-157: Multi-line formatting for ternary operation
  • Lines 163: Consistent empty function formatting
  • Lines 229-239: Improved readability of nested function calls with proper indentation

🔍 Code Quality Assessment

No issues found with:

  • Bugs: No functional changes that could introduce bugs
  • Security concerns: No security-related changes
  • Performance: No performance impact (formatting only)
  • Test coverage: No test changes needed (formatting only)

✅ Best Practices

The changes follow project conventions from CLAUDE.md:

  • Using Node.js import protocol convention
  • Proper code formatting (though note: CLAUDE.md says not to run cargo fmt automatically, but this is TypeScript/Biome formatting which is separate)

Recommendation

✅ APPROVE - This is a clean formatting PR that:

  • Improves code consistency
  • Follows project linting rules
  • Makes the codebase more maintainable
  • Contains no functional changes that could introduce bugs

The changes are safe to merge.

@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 5 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): fmt

Overview

This PR applies formatting changes to TypeScript files in the engine/sdks directory by enabling Biome formatter for this path. The changes are primarily cosmetic, reformatting code to comply with Biome's style rules.

Positive Aspects

Formatting Consistency: The changes improve code consistency across the TypeScript SDK files
Import Protocol Compliance: The change in log.ts properly uses node:util instead of just util, which aligns with Biome's useNodejsImportProtocol rule
No Functional Changes: All modifications are purely formatting-related with no logic changes
Scope Expansion: Enabling Biome for engine/sdks (except specific exclusions) will ensure future commits maintain consistent formatting

Code Quality Observations

biome.json (engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:28-35, :440-448)

  • Parameter formatting is improved with better readability for multi-parameter functions
  • Consistent indentation and line breaks follow Biome's style

engine/sdks/typescript/test-runner/src/index.ts

  • Line 15: Proper single-line formatting for simple assignments
  • Lines 107-108: else statement correctly moved to same line per style guide
  • Lines 156-157, 229-239: Better readability with proper line breaks for complex expressions
  • Empty function bodies (line 163) now properly formatted as () => {}

engine/sdks/typescript/test-runner/src/log.ts

  • Line 1: Import changed from "util" to "node:util" - this is the correct Node.js protocol per Biome's useNodejsImportProtocol rule

Potential Concerns

⚠️ Scope of Change: Removing "!engine/sdks" from biome.json means all SDK code will now be subject to Biome formatting. This is likely intentional, but ensure:

  • All team members are aware of this change
  • CI/CD pipelines enforce Biome formatting for these files
  • The exclusions for api-full and runner-protocol are intentional (they appear to be generated code)

Testing Considerations

Since this is purely a formatting change:

  • ✅ No new tests are needed
  • ✅ Existing tests should continue to pass
  • ⚠️ Consider running a full test suite to ensure no accidental syntax issues were introduced

Recommendations

  1. CI Integration: Ensure Biome formatting is enforced in CI for the newly included paths
  2. Team Communication: Make sure the team knows that engine/sdks is now under Biome formatting control
  3. Pre-commit Hooks: Consider adding Biome format checks to pre-commit hooks if not already present

Security & Performance

  • ✅ No security concerns
  • ✅ No performance impact

Verdict

LGTM

This is a clean formatting PR that improves code consistency. The changes follow Biome's style guide correctly and include a beneficial update to use the Node.js import protocol for the util module.


🤖 Generated with Claude Code

@NathanFlurry NathanFlurry force-pushed the 11-03-chore_rivetkit_fmt branch from 19cd542 to 4f8383d Compare November 4, 2025 19:24
@NathanFlurry NathanFlurry force-pushed the 11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs branch from 8d1d88d to 35bf113 Compare November 4, 2025 19:24
@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

This PR applies code formatting changes using Biome to the TypeScript SDK files. Overall, the changes are straightforward and improve code consistency. Here's my detailed review:

Positive Aspects

  1. Biome Configuration Update: The removal of !engine/sdks exclusion in biome.json:9 is appropriate, allowing Biome to format SDK code while still excluding generated code (api-full, runner-protocol).

  2. Consistent Formatting: All formatting changes follow TypeScript/JavaScript best practices:

    • Multi-line function parameters with proper indentation
    • Consistent spacing around operators
    • Proper else spacing (} else instead of }\n else)
    • Correct Node.js import protocol usage (node:util instead of util)
  3. No Functional Changes: This is purely a formatting PR with no behavior modifications, reducing risk.

📝 Code Quality Observations

websocket-tunnel-adapter.ts (lines 31-35, 443-448):

  • The multi-line formatting of long function signatures improves readability
  • Parameters are properly aligned and indented

test-runner/src/index.ts:

  • Line 15: Proper formatting of RIVET_RUNNER_KEY declaration
  • Lines 107-108: Correctly formatted else statement
  • Lines 156-157: Better line breaking for runnerKey property
  • Lines 229-239: Improved readability of chained method calls with proper indentation

test-runner/src/log.ts (line 1):

  • ✅ Correctly uses Node.js import protocol: import { inspect } from "node:util"
  • This aligns with the Biome rule useNodejsImportProtocol: "error" in biome.json:34

🔍 Recommendations & Considerations

  1. Testing: Since these are formatting-only changes, no new tests are needed. However, consider:

    • Running existing tests to ensure no unintended side effects
    • Verifying the test-runner still functions correctly
  2. CI/CD Integration: Consider adding a Biome check to CI to prevent future formatting inconsistencies:

    biome check --apply-unsafe
  3. Documentation: The commit message chore(rivetkit): fmt follows the conventional commits format mentioned in CLAUDE.md, which is good.

  4. Consistency: Now that engine/sdks is included in Biome's scope, ensure all future SDK changes maintain this formatting standard.

🎯 Verdict

LGTM

This PR successfully applies consistent formatting to the TypeScript SDK files without introducing any functional changes. The changes align with TypeScript best practices and Biome's configuration.

Recommendation: Approve and merge.


Review generated with Claude Code

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(rivetkit): fmt

Overview

This PR applies automated formatting and linting fixes to TypeScript SDK files, primarily enabling biome linting by removing the !engine/sdks exclusion from biome.json.

✅ Positive Observations

  1. Consistent Formatting: All changes follow standard formatting conventions:

    • Multi-line function parameters are properly aligned
    • Proper use of trailing commas
    • Consistent spacing and indentation
  2. Import Protocol Compliance: The change in log.ts from import { inspect } from "util" to import { inspect } from "node:util" aligns with the biome rule useNodejsImportProtocol (set to "error" in biome.json:34). This is a best practice for Node.js imports.

  3. No Functional Changes: All changes are purely cosmetic formatting fixes that don't alter runtime behavior.

🔍 Code Quality & Best Practices

Good:

  • The formatting improves readability, especially for complex function signatures like _handleMessage and the constructor in websocket-tunnel-adapter.ts
  • Empty arrow functions are now consistently formatted as () => {}
  • Removed unnecessary line breaks in constant declarations

Suggestions:
None - this is a straightforward formatting PR that follows established conventions.

🐛 Potential Issues

No issues found. The changes are safe and automated.

⚡ Performance Considerations

No performance impact - these are purely formatting changes.

🔒 Security Concerns

No security concerns identified.

🧪 Test Coverage

Note: While there are existing tests in engine/sdks/typescript/runner/tests/lifecycle.test.ts, this PR only modifies formatting and doesn't require additional test coverage. The changes are low-risk.

Recommendation: Consider running the existing test suite to ensure formatting changes don't inadvertently break anything (though this is highly unlikely):

# From the runner directory
npm test
# or
pnpm test

📝 Additional Notes

  1. biome.json Change: Removing !engine/sdks from the ignore list means more files will be linted going forward. This is good for code quality consistency.

  2. Files Still Excluded: The specific exclusions for api-full and runner-protocol remain, which is appropriate as these are likely generated files.

  3. Commit Message: The commit message chore(rivetkit): fmt follows conventional commit format, though it would be slightly more accurate as chore(engine/sdks): fmt since the changes are in the engine/sdks directory rather than rivetkit specifically.

✨ Verdict

LGTM

This is a clean, low-risk formatting PR that improves code consistency. No changes requested.


Review generated by Claude Code

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:10 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:53 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:53 PM UTC: CI is running for this pull request on a draft pull request (#3353) due to your merge queue CI optimization settings.
  • Nov 4, 8:56 PM UTC: Merged by the Graphite merge queue via draft PR: #3353.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@claude
Copy link

claude bot commented Nov 4, 2025

Code Review - PR #3331: chore(rivetkit): fmt

Summary

This PR applies formatting changes to TypeScript files in the RivetKit engine SDKs using Biome. The changes are primarily stylistic and include:

  1. Enabling Biome formatting for engine/sdks directory
  2. Formatting function signatures with better line breaks
  3. Standardizing spacing and import statements

Detailed Review

Positive Aspects

  1. Consistent Formatting: The changes apply consistent formatting rules across the codebase, improving readability
  2. Better Function Signatures: Multi-parameter function signatures are now properly broken across lines (e.g., closeCallback in websocket-tunnel-adapter.ts:31-35)
  3. Import Organization: The change from import { inspect } from "util" to import { inspect } from "node:util" follows the useNodejsImportProtocol rule (biome.json:34), which is a Node.js best practice

📝 Observations & Suggestions

  1. Biome Configuration Change (biome.json:8)

    • Change: Removed "!engine/sdks" from the exclude list
    • Impact: This now enables Biome formatting for all TypeScript SDK files
    • Question: Is this intentional to format all SDK files, or should specific generated SDK directories remain excluded? I notice api-full and runner-protocol are still excluded, which suggests generated code is intentionally skipped.
    • Recommendation: ✅ This looks correct - it enables formatting for hand-written SDK code while preserving exclusions for generated code
  2. Code Quality - No Logic Changes

    • ✅ All changes are purely formatting - no logic modifications detected
    • This is ideal for a formatting PR and reduces risk
  3. Empty Callback Function (test-runner/src/index.ts:162)

    • Before: onDisconnected: () => { },
    • After: onDisconnected: () => {},
    • Note: This is still an empty callback. Consider whether this should have any cleanup logic or at least a comment explaining why it's intentionally empty
  4. Line Break Consistency (test-runner/src/index.ts:14-15)

    • The formatting now properly handles the ternary operator across multiple lines
    • ✅ Improved readability

🧪 Testing Considerations

  • Impact: Zero runtime impact expected - these are pure formatting changes
  • Recommendation: Standard CI checks should be sufficient
  • Build: Ensure TypeScript compilation succeeds (should be automatic in CI)

🔒 Security Considerations

  • ✅ No security concerns - formatting changes only

📊 Performance Considerations

  • ✅ No performance impact - code logic unchanged

🎯 Best Practices Alignment

According to CLAUDE.md:

  • ✅ Follows repository's formatting standards
  • ✅ Uses Node.js import protocol (node:util instead of util)
  • ✅ Proper use of formatting tools

Overall Assessment

Status: ✅ Approved

This is a clean formatting PR that:

  • Applies consistent code style across TypeScript SDK files
  • Enables Biome formatting for the appropriate directories
  • Makes no logic changes that could introduce bugs
  • Improves code readability

Recommendation: Merge after CI passes. The changes align with repository standards and improve code consistency.

Minor Suggestions (Non-blocking)

  1. Consider adding a comment to the empty onDisconnected callback if it's intentionally a no-op
  2. Verify that all intended SDK directories should now be formatted (the biome.json change enables formatting for previously excluded paths)

🤖 Review generated by Claude Code

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-03-chore_rivetkit_fmt branch November 4, 2025 20:56
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