fix(core): fix json encoding #1336

NathanFlurry · 2025-10-03T04:41:13Z

No description provided.

vercel · 2025-10-03T04:41:17Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Oct 7, 2025 11:38pm

claude · 2025-10-03T04:41:26Z

Claude finished @NathanFlurry's task —— View job

PR Review: fix(core): fix json encoding

Comprehensive review completed. This PR introduces a robust JSON encoding/decoding system with proper escaping for special types.

Todo List:

Read and analyze all changed files
Review JSON encoding implementation changes
Analyze new test coverage
Check adherence to repository conventions
Provide comprehensive feedback

✅ Strengths

1. Robust Type System Support

Excellent implementation of jsonStringifyCompat and jsonParseCompat functions in packages/rivetkit/src/actor/protocol/serde.ts:147-203
Proper handling of special types: BigInt, ArrayBuffer, and Uint8Array with base64 encoding
Smart escaping system for user arrays starting with $ to avoid conflicts with internal type markers

2. Comprehensive Test Coverage

New test file packages/rivetkit/src/json-escaping.test.ts provides excellent coverage (221 lines)
Tests cover all edge cases: empty objects, null values, undefined values, nested structures, and error conditions
Particularly good testing of the escaping mechanism for user data that could conflict with internal type markers

3. Clean Architecture

Proper separation of concerns with dedicated functions for encoding/decoding
Integration with existing serializeWithEncoding and deserializeWithEncoding functions
Cross-platform compatibility for both Node.js and browser environments (base64 handling)

⚠️ Areas for Improvement

1. Error Handling

In packages/rivetkit/src/actor/protocol/serde.ts:196-198, the error message could be more specific about potential causes:

throw new Error(
    `Unknown JSON encoding type: ${value[0]}. This may indicate corrupted data or a version mismatch.`,
);

Consider adding guidance on how to resolve this issue or what versions are compatible.

2. Performance Considerations

The escaping mechanism in jsonStringifyCompat adds overhead by checking every array. For high-throughput applications, consider:
- Caching escaped values where possible
- Adding performance benchmarks to ensure acceptable overhead
- Documentation about performance implications

3. Documentation Gap

Missing JSDoc comments for the new public functions jsonStringifyCompat and jsonParseCompat
Per repository guidelines, public APIs should have proper JSDoc comments

🔒 Security Assessment

Positive Security Aspects:

Input validation prevents code injection through type markers
Base64 encoding for binary data is safe and standard
Error throwing on unknown types prevents silent data corruption

Recommendations:

Consider adding input size limits for very large payloads
The base64 encoding is secure but adds ~33% size overhead for binary data

🚀 Performance Analysis

Efficient Design:

Uses native JSON.stringify/JSON.parse as the foundation
Base64 operations are optimized for both Node.js and browser environments
Minimal memory allocations in the replacer/reviver functions

Potential Optimizations:

For frequently serialized objects, consider implementing a cache for escaped arrays
Binary type detection could be optimized with instanceof checks before string operations

🧪 Test Quality

Excellent Coverage:

Tests thoroughly cover the escaping/unescaping mechanism
Edge cases well tested (empty objects, null values, deeply nested structures)
Error conditions properly validated
Integration with existing driver test suite ensures compatibility

📋 Minor Issues

Type Safety: Consider adding more specific types for the special type markers to improve IntelliSense support
Driver Test Suite: The changes to packages/rivetkit/src/driver-test-suite/mod.ts and utils.ts appear minimal and appropriate for supporting the new encoding system
Consistency: The implementation follows repository conventions well (lowercase logging, proper error handling, TypeScript best practices)

🎯 Overall Assessment

This is a high-quality PR that addresses JSON encoding issues with a well-thought-out approach. The escaping mechanism is clever and prevents conflicts between user data and internal type markers. The comprehensive test suite demonstrates thorough consideration of edge cases.

Recommendation: APPROVE with minor suggestions for documentation improvements.

Priority Actions:

Add JSDoc comments to public functions
Consider adding performance notes to documentation
Minor error message improvements

The implementation is production-ready and significantly improves the robustness of JSON serialization in the system.

NathanFlurry · 2025-10-03T04:41:30Z

chore(core): remove unused bufferutil #1337
fix(core): fix json encoding #1336 👈 (View in Graphite)
main

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.

pkg-pr-new · 2025-10-03T04:43:31Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: bec1ee6

graphite-app · 2025-10-07T23:38:35Z

Merge activity

Oct 7, 11:38 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Oct 7, 11:39 PM UTC: CI is running for this pull request on a draft pull request (#1366) due to your merge queue CI optimization settings.
Oct 7, 11:39 PM UTC: Merged by the Graphite merge queue via draft PR: #1366.

claude · 2025-10-07T23:38:41Z

Claude finished @NathanFlurry's task —— View job

PR Review: fix(core): fix json encoding

Comprehensive review completed. This PR introduces a robust JSON encoding/decoding system with proper escaping for special types.

Todo List:

Read repository guidelines (CLAUDE.md)
Read and analyze all changed files
Review JSON encoding implementation changes
Analyze new test coverage
Check for potential bugs and security issues
Assess performance implications
Check adherence to repository conventions
Provide comprehensive feedback

✅ Strengths

1. Robust Type System Support

Excellent implementation of jsonStringifyCompat and jsonParseCompat functions in packages/rivetkit/src/actor/protocol/serde.ts:147-203
Proper handling of special types: BigInt, ArrayBuffer, and Uint8Array with base64 encoding
Smart escaping system for user arrays starting with $ to avoid conflicts with internal type markers

2. Comprehensive Test Coverage

New test file packages/rivetkit/tests/json-escaping.test.ts provides excellent coverage (221 lines)
Tests cover all edge cases: empty objects, null values, undefined values, nested structures, and error conditions
Particularly good testing of the escaping mechanism for user data that could conflict with internal type markers

3. Clean Architecture

Proper separation of concerns with dedicated functions for encoding/decoding
Integration with existing serializeWithEncoding and deserializeWithEncoding functions in packages/rivetkit/src/serde.ts:61, 81
Cross-platform compatibility for both Node.js and browser environments (base64 handling)

⚠️ Areas for Improvement

1. Error Handling

In packages/rivetkit/src/actor/protocol/serde.ts:196-198, the error message could be more specific about potential causes:

throw new Error(
    `Unknown JSON encoding type: ${value[0]}. This may indicate corrupted data or a version mismatch.`,
);

Consider adding guidance on how to resolve this issue or what versions are compatible.

2. Performance Considerations

The escaping mechanism in jsonStringifyCompat adds overhead by checking every array. For high-throughput applications, consider:
- Caching escaped values where possible
- Adding performance benchmarks to ensure acceptable overhead
- Documentation about performance implications

3. Documentation Gap

Missing JSDoc comments for the new public functions jsonStringifyCompat and jsonParseCompat
Per repository guidelines, public APIs should have proper JSDoc comments

🔒 Security Assessment

Positive Security Aspects:

Input validation prevents code injection through type markers
Base64 encoding for binary data is safe and standard
Error throwing on unknown types prevents silent data corruption

Recommendations:

Consider adding input size limits for very large payloads
The base64 encoding is secure but adds ~33% size overhead for binary data

🚀 Performance Analysis

Efficient Design:

Uses native JSON.stringify/JSON.parse as the foundation
Base64 operations are optimized for both Node.js and browser environments
Minimal memory allocations in the replacer/reviver functions

Potential Optimizations:

For frequently serialized objects, consider implementing a cache for escaped arrays
Binary type detection could be optimized with instanceof checks before string operations

🧪 Test Quality

Excellent Coverage:

Tests thoroughly cover the escaping/unescaping mechanism
Edge cases well tested (empty objects, null values, deeply nested structures)
Error conditions properly validated
Integration with existing driver test suite ensures compatibility

📋 Minor Issues

Type Safety: Consider adding more specific types for the special type markers to improve IntelliSense support
Driver Test Suite: The changes to packages/rivetkit/src/driver-test-suite/mod.ts and utils.ts appear minimal and appropriate for supporting the new encoding system
Consistency: The implementation follows repository conventions well (lowercase logging, proper error handling, TypeScript best practices)

🎯 Overall Assessment

This is a high-quality PR that addresses JSON encoding issues with a well-thought-out approach. The escaping mechanism is clever and prevents conflicts between user data and internal type markers. The comprehensive test suite demonstrates thorough consideration of edge cases.

Recommendation: APPROVE with minor suggestions for documentation improvements.

Priority Actions:

Add JSDoc comments to public functions
Consider adding performance notes to documentation
Minor error message improvements

The implementation is production-ready and significantly improves the robustness of JSON serialization in the system.

vercel bot deployed to Preview October 3, 2025 04:42 View deployment

NathanFlurry mentioned this pull request Oct 3, 2025

chore(core): remove unused bufferutil #1337

Closed

fix(core): fix json encoding

0655a15

NathanFlurry force-pushed the 10-02-fix_core_fix_json_encoding branch from bec1ee6 to 0655a15 Compare October 7, 2025 23:38

vercel bot had a problem deploying to Preview October 7, 2025 23:38 Failure

graphite-app bot pushed a commit that referenced this pull request Oct 7, 2025

fix(core): fix json encoding (#1336)

9d291ed

graphite-app bot closed this Oct 7, 2025

graphite-app bot deleted the 10-02-fix_core_fix_json_encoding branch October 7, 2025 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(core): fix json encoding #1336

fix(core): fix json encoding #1336

NathanFlurry commented Oct 3, 2025

Uh oh!

vercel bot commented Oct 3, 2025 •

edited

Loading

Uh oh!

claude bot commented Oct 3, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Oct 3, 2025 •

edited

Loading

Uh oh!

pkg-pr-new bot commented Oct 3, 2025

Uh oh!

graphite-app bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

claude bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

fix(core): fix json encoding #1336

fix(core): fix json encoding #1336

Conversation

NathanFlurry commented Oct 3, 2025

Uh oh!

vercel bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: fix(core): fix json encoding

Todo List:

✅ Strengths

⚠️ Areas for Improvement

🔒 Security Assessment

🚀 Performance Analysis

🧪 Test Quality

📋 Minor Issues

🎯 Overall Assessment

Uh oh!

NathanFlurry commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

pkg-pr-new bot commented Oct 3, 2025

Uh oh!

graphite-app bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

claude bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: fix(core): fix json encoding

Todo List:

✅ Strengths

⚠️ Areas for Improvement

🔒 Security Assessment

🚀 Performance Analysis

🧪 Test Quality

📋 Minor Issues

🎯 Overall Assessment

Uh oh!

Uh oh!

vercel bot commented Oct 3, 2025 •

edited

Loading

claude bot commented Oct 3, 2025 •

edited

Loading

NathanFlurry commented Oct 3, 2025 •

edited

Loading

graphite-app bot commented Oct 7, 2025 •

edited

Loading

claude bot commented Oct 7, 2025 •

edited

Loading