Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 23, 2025

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

3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 24, 2025 8:41pm
rivet-inspector Ignored Ignored Preview Oct 24, 2025 8:41pm
rivet-site Ignored Ignored Preview Oct 24, 2025 8:41pm

Copy link
Member Author

NathanFlurry commented Oct 23, 2025


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 23, 2025

PR Review

Summary

This PR fixes the Biome configuration to properly ignore the engine/artifacts directory. The changes update the ignore path pattern and apply formatting changes to the error JSON files in that directory.

Code Quality - Approved

Positive aspects:

  • Clear and focused change with a single purpose
  • The fix correctly updates the ignore path from packages/engine/artifacts to engine/artifacts, which aligns with the actual directory structure
  • Alphabetizes the includes list for better maintainability

Observations:

  • The formatting changes to the JSON files (tabs to spaces, removing trailing newlines) suggest these files were previously formatted but are now being reformatted due to the corrected ignore path
  • This is expected behavior after fixing the ignore pattern

Best Practices - Approved

The changes follow repository conventions:

  • Uses proper JSON formatting with 2-space indentation
  • Maintains consistent structure across all error definition files
  • The biome.json configuration is now more maintainable with alphabetized paths

Potential Issues - Minor

Minor concern:
The JSON files now lack trailing newlines. While this is cosmetic, it is generally considered good practice to include trailing newlines in text files for better POSIX compliance and cleaner diffs.

Question:
Since engine/artifacts is now being ignored by Biome, these formatting changes will be the last ones applied. Future changes to these files will not be automatically formatted. Is this intentional? If these are generated artifacts, ignoring them makes sense. If they are manually maintained, you might want Biome to continue formatting them.

Security - No Issues

No security concerns identified. This is a configuration and formatting change with no functional impact.

Performance - No Impact

No performance implications. These are configuration and formatting changes only.

Test Coverage - N/A

No tests needed for this change. The fix can be verified by:

  1. Running Biome and confirming it no longer tries to format files in engine/artifacts/
  2. Verifying the corrected path matches the actual directory structure

Recommendations

  1. Consider: If engine/artifacts contains generated files that should never be manually edited, consider adding a comment in biome.json explaining this

  2. Optional: If you want consistency, you could add trailing newlines back to the JSON files, though this is purely cosmetic

  3. Documentation: If these artifact files are generated, it might be worth documenting the generation process in a README or CLAUDE.md

Overall Assessment

This is a well-executed fix that correctly addresses the ignore path issue. The PR is focused, the changes are appropriate, and there are no functional concerns.

Recommendation: Approve and Merge

The formatting changes are a natural consequence of fixing the ignore path, and the configuration update properly reflects the repository structure.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 23, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 323a773

@NathanFlurry NathanFlurry force-pushed the 10-22-chore_fix_rivetkit_release_script branch from 4b9f24b to f28b056 Compare October 24, 2025 06:32
@NathanFlurry NathanFlurry force-pushed the 10-23-fix_fix_biome_ignore_path branch from 7b4e5e7 to 323a773 Compare October 24, 2025 06:32
@claude
Copy link

claude bot commented Oct 24, 2025

PR Review: Fix biome ignore path

🚨 Critical Issue - JSON Syntax Error

There is a missing comma on line 11 after "!engine/sdks/typescript/runner-protocol" which makes this JSON file invalid:

"!engine/sdks/typescript/runner-protocol"
"!examples/snippets",  // <-- Missing comma on the line above

This will cause biome to fail when it tries to parse this configuration file.

Code Quality Observations

Positive Changes:

  • ✅ Alphabetizing the ignore patterns improves maintainability
  • ✅ Moving **/*.js before **/*.json creates a consistent alphabetical ordering for file extensions
  • ✅ Adding the more general !engine/sdks pattern is good for catching all SDK directories

Suggestions:

  1. Fix the syntax error by adding a comma after line 11
  2. Consider redundancy: The pattern !engine/sdks on line 9 will already match both !engine/sdks/typescript/api-full and !engine/sdks/typescript/runner-protocol on lines 10-11. These specific patterns may be redundant unless there are other directories under engine/sdks/ that you want to include.

Testing Recommendation

After fixing the syntax error, verify the configuration works:

npx @biomejs/biome check --config-path=biome.json

Summary

Status: ❌ Needs changes (syntax error must be fixed)

Priority: High - The missing comma will break biome entirely

Action Items:

  1. Add comma after "!engine/sdks/typescript/runner-protocol" on line 11
  2. Consider removing redundant patterns on lines 10-11 if line 9's !engine/sdks pattern covers them

@claude
Copy link

claude bot commented Oct 24, 2025

PR Review: Fix biome ignore path

Summary

This PR fixes a syntax error in biome.json and reorganizes the ignore patterns for better clarity.

Issues Found

Critical Issue - Missing Comma ⚠️

The current merged version has a syntax error on line 11:

"!engine/sdks/typescript/runner-protocol"
"!examples/snippets",

There's a missing comma after "!engine/sdks/typescript/runner-protocol" (line 11), which makes this invalid JSON. This would cause biome to fail to parse its configuration file.

Changes Review

Positive Changes:

  1. ✅ Adds !engine/sdks as a broader ignore pattern (line 9)
  2. ✅ Alphabetizes the ignore patterns for better maintainability
  3. ✅ Groups related patterns together (engine-related ignores first, then alphabetical project ignores)

Concerns:

  1. 🔴 The merged code still has the missing comma syntax error - this needs to be fixed immediately

  2. ⚠️ The new !engine/sdks pattern (line 9) overlaps with the existing specific patterns:

    • !engine/sdks/typescript/api-full (line 10)
    • !engine/sdks/typescript/runner-protocol (line 11)

    Since !engine/sdks excludes the entire directory, the more specific patterns are redundant. Consider removing lines 10-11 if the intent is to ignore all of engine/sdks.

Recommendations

  1. Immediate fix required: Add the missing comma on line 11
  2. Simplification opportunity: If all of engine/sdks should be ignored, remove the redundant specific patterns
  3. Consider documenting why certain paths are ignored (especially the newer !engine/sdks pattern) for future maintainers

Code Quality

  • Configuration organization: Good (alphabetical sorting)
  • Syntax: Failed (missing comma)
  • Pattern efficiency: Could be improved (redundant patterns)

Status: ❌ Requires immediate fix for the syntax error


Review generated via Claude Code

@NathanFlurry NathanFlurry force-pushed the 10-23-fix_fix_biome_ignore_path branch from 323a773 to a97aa78 Compare October 24, 2025 20:40
@NathanFlurry NathanFlurry force-pushed the 10-22-chore_fix_rivetkit_release_script branch from f28b056 to 5b91eee Compare October 24, 2025 20:40
@claude
Copy link

claude bot commented Oct 24, 2025

PR Review: Fix biome ignore path

Summary

This PR reorganizes the biome.json configuration file by alphabetically sorting the ignore paths and grouping them more logically.

Issues Found

⚠️ Critical: JSON Syntax Error

The current state of biome.json (line 11) is missing a comma after "!engine/sdks/typescript/runner-protocol", which will cause a JSON parse error:

"!engine/sdks/typescript/api-full",
"!engine/sdks/typescript/runner-protocol"  // ❌ Missing comma here
"!examples/snippets",

This should be:

"!engine/sdks/typescript/api-full",
"!engine/sdks/typescript/runner-protocol",  // ✅ Comma added
"!examples/snippets",

Feedback

Positive:

  • ✅ Good intent to organize and clean up the ignore patterns
  • ✅ Alphabetical sorting of ignore paths improves maintainability
  • ✅ Moving **/*.js before **/*.json provides better logical grouping

Issues:

  • ❌ The JSON syntax error will break biome functionality
  • ⚠️ The reorganization changes might have unintended consequences if biome respects order precedence (though unlikely for simple path patterns)

Recommendations:

  1. Fix the syntax error by adding the missing comma on line 11
  2. Consider testing that biome still works correctly after this change
  3. Validate the JSON with a linter before merging

Severity

🔴 High - This PR introduces a breaking syntax error that will prevent biome from functioning.


Review generated by Claude Code

@claude
Copy link

claude bot commented Oct 24, 2025

Pull Request Review

Summary

This PR reorganizes the Biome configuration ignore paths and fixes a critical syntax error.


✅ Positive Findings

  1. Alphabetical Organization - The ignore paths are now alphabetically sorted, which improves maintainability
  2. Broader SDK Exclusion - Adding !engine/sdks provides a more comprehensive exclusion that covers all SDK directories

🐛 Critical Issue Found

Missing Comma on Line 11 ⚠️

There's a syntax error in the current merged code at biome.json:11. The line:

"!engine/sdks/typescript/runner-protocol"

is missing a trailing comma before the next item. This will cause JSON parsing to fail.

Expected:

"!engine/sdks/typescript/runner-protocol",

🔍 Code Quality Observations

Redundant Patterns:

  • Line 9: !engine/sdks (excludes all SDKs)
  • Lines 10-11: !engine/sdks/typescript/api-full and !engine/sdks/typescript/runner-protocol (specific TypeScript SDK exclusions)

Since !engine/sdks already excludes the entire engine/sdks/ directory tree, the specific TypeScript paths on lines 10-11 are redundant. Consider removing them for cleaner configuration:

"includes": [
  "**/*.js",
  "**/*.json",
  "**/*.ts",
  "!engine/artifacts",
  "!engine/sdks",
  "!examples/snippets",
  "!frontend",
  "!rivetkit-openapi/openapi.json",
  "!scripts",
  "!website"
],

📝 Recommendations

  1. Immediate: Fix the missing comma on line 11
  2. Optional: Remove redundant TypeScript SDK exclusions since !engine/sdks covers them
  3. Testing: Verify Biome runs successfully after this change with biome check .

Security & Performance

  • ✅ No security concerns
  • ✅ No performance impact (configuration file only)

Test Coverage

  • ⚠️ Consider adding a CI check to validate JSON syntax in configuration files to catch issues like this automatically

Overall Assessment: The organizational improvements are good, but the missing comma is a blocking issue that needs immediate attention.

Base automatically changed from 10-22-chore_fix_rivetkit_release_script to main October 25, 2025 09:20
@NathanFlurry NathanFlurry merged commit 43441a8 into main Oct 25, 2025
9 of 17 checks passed
@NathanFlurry NathanFlurry deleted the 10-23-fix_fix_biome_ignore_path branch October 25, 2025 09:21
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.

2 participants