Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 23, 2025

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

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 2, 2026 11:34pm
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 2, 2026 11:34pm
rivet-inspector Ignored Ignored Preview Jan 2, 2026 11:34pm
rivet-site Ignored Ignored Preview Jan 2, 2026 11:34pm

Copy link
Member Author

NathanFlurry commented Dec 23, 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 Dec 23, 2025

PR Review: fix(rivetkit): fix c.client not respecting correct origin

Summary

This PR contains only code formatting changes to rivetkit-typescript/packages/rivetkit/src/manager/router.ts. The changes reformat three multi-line function calls to improve code readability and consistency.

Changes Analysis

The PR modifies 3 locations:

  1. Line 168: Reformatted Object.keys(config.use) to single line
  2. Line 438-440: Reformatted createTestWebSocketProxy() call to multi-line
  3. Line 558: Reformatted modifyManagerRouter?.() call to single line

Issues Identified

⚠️ Critical: Title Mismatch

The PR title claims to fix c.client not respecting the correct origin, but no functional changes are present. The diff shows only formatting adjustments with no logic changes that would affect client origin handling.

Recommendation: Either:

  • Update the PR title to reflect that this is a formatting/style fix (e.g., chore(rivetkit): format router.ts)
  • OR add the actual origin fix if it's missing from this PR

Code Quality Assessment

✅ Positives

  • Formatting changes improve code consistency
  • No functional logic altered (reduces risk)
  • Changes align with common TypeScript formatting conventions

⚠️ Concerns

  1. Missing Actual Fix: If there truly is a bug with c.client origin handling, it's not addressed in this PR
  2. Formatting Tool: According to CLAUDE.md, the project uses cargo fmt for Rust but this is TypeScript - consider using prettier or eslint with --fix consistently across the codebase
  3. Incomplete Context: Without seeing the actual origin bug, it's unclear if this PR solves the stated problem

Testing

  • ⚠️ No tests added or modified
  • Since these are formatting-only changes, existing tests should pass
  • However, if there's an actual origin bug, there should be tests validating the fix

Recommendations

  1. Clarify Intent: Update PR title and description to accurately reflect what this PR does
  2. Add Functional Changes: If the origin bug is real, add the actual fix
  3. Add Tests: Include tests that verify the origin is correctly respected in c.client calls
  4. Document Root Cause: Explain what the origin bug was and how it manifested

Security & Performance

  • ✅ No security concerns (formatting only)
  • ✅ No performance impact

Verdict

REQUEST CHANGES - The PR title doesn't match the changes. Please clarify whether this is:

  • A formatting-only PR (update title)
  • Missing the actual bug fix (add the fix)

Review generated by Claude Code

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 1, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: bf7663e

@NathanFlurry NathanFlurry force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from bf7663e to 81d8c74 Compare January 2, 2026 22:30
@NathanFlurry NathanFlurry force-pushed the 12-22-fix_rivetkit_allow_providing_no_args_to_createrivetkit branch from a2d24db to 258b84d Compare January 2, 2026 22:30
@claude
Copy link

claude bot commented Jan 2, 2026

PR Review - Update

After reviewing the latest changes to PR #3668, I have the following observations:

Critical Issue: Misleading PR Title

The PR title claims to fix c.client not respecting correct origin, but the actual changes are purely cosmetic formatting changes.

The diff shows:

  • Line 168: Reformatting Object.keys(config.use) call to single line
  • Line 438-440: Reformatting createTestWebSocketProxy() call to multi-line
  • Line 558: Reformatting managerDriver.modifyManagerRouter?.() call to single line

None of these changes affect functionality or fix any origin-related issue with c.client.

Analysis

  1. Misleading Documentation: The PR title and commit message describe a functional fix that doesn't exist in the code changes
  2. Formatting Only: All changes are whitespace/line-break adjustments that don't alter behavior
  3. No Test Changes: No tests added or modified to verify the claimed fix
  4. Missing Context: This is a stacked PR (merging into PR fix(rivetkit): allow providing no args to createRivetKit #3667), but even considering that context, there's no functional change here

Questions for the Author

  1. Is the PR title correct? If this is meant to fix an origin issue, where is the actual fix?
  2. Should this be merged with another PR? If the fix is in the base branch (PR fix(rivetkit): allow providing no args to createRivetKit #3667), this PR might be unnecessary
  3. Is this just a formatting PR? If so, the title should be updated to reflect that (e.g., chore(rivetkit): format manager router code)

Recommendations

Do not merge this PR as-is. Either:

  1. Add the actual functional fix described in the title, or
  2. Update the title to accurately reflect that this is a formatting-only change, or
  3. Close this PR if the changes aren't needed

Note on Development Process

According to CLAUDE.md:

DO NOT RUN CARGO FMT AUTOMATICALLY

While this is TypeScript not Rust, the principle suggests formatting changes should be intentional and batched appropriately, not mixed with functional fixes.


Review updated: 2026-01-03

@jog1t jog1t force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from 81d8c74 to 49e0689 Compare January 2, 2026 23:33
@jog1t jog1t force-pushed the 12-22-fix_rivetkit_allow_providing_no_args_to_createrivetkit branch from 258b84d to 53631b6 Compare January 2, 2026 23:33
@NathanFlurry NathanFlurry force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from 49e0689 to 81d8c74 Compare January 2, 2026 23:59
@NathanFlurry NathanFlurry force-pushed the 12-22-fix_rivetkit_allow_providing_no_args_to_createrivetkit branch from 53631b6 to 258b84d Compare January 2, 2026 23:59
@claude
Copy link

claude bot commented Jan 3, 2026

PR Review

Summary

This PR claims to fix c.client not respecting the correct origin, but the changes only contain code formatting updates. There are no functional changes that would address the issue described in the title.

Issues Found

🚨 Critical: Mismatch between PR title and changes

  • PR Title: "fix(rivetkit): fix c.client not respecting correct origin"
  • Actual Changes: Only formatting/prettier changes (line breaks and indentation)
  • Impact: The claimed bug fix is not present in this PR

The diff shows only these formatting changes:

  1. Lines 168-170: Reformatted for loop to single line
  2. Lines 438-440: Reformatted createTestWebSocketProxy call with line breaks
  3. Line 558: Reformatted modifyManagerRouter call to single line

None of these changes affect how c.client handles origins or any client-related functionality.

Questions for Author

  1. Is this the correct PR? The changes don't match the description at all.
  2. Where is the actual fix? If there's a bug with c.client and origin handling, what changes address it?
  3. Should this be closed? If this is just an accidental formatting-only PR, it might be worth closing and creating the real fix.

Code Quality Notes

The formatting changes themselves are fine and appear to match the project's style guidelines. However:

  • ⚠️ Per CLAUDE.md: "Do not run ./scripts/cargo/fix.sh. Do not format the code yourself."
  • These changes look like manual formatting or an auto-formatter was run
  • The project prefers formatting to be handled by pre-commit hooks when everything is merged

Recommendation

Do not merge until the actual bug fix is included or the PR title/description is corrected to reflect that this is just a formatting change.

If this PR is meant to fix an actual bug with c.client and origins, please:

  1. Add the functional changes that fix the bug
  2. Add tests to verify the fix
  3. Update the PR description to explain what the bug was and how it's fixed

Review generated by Claude Code

@jog1t jog1t force-pushed the 12-22-fix_rivetkit_allow_providing_no_args_to_createrivetkit branch from 258b84d to 53631b6 Compare January 3, 2026 00:05
@jog1t jog1t force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from 81d8c74 to 49e0689 Compare January 3, 2026 00:05
@claude
Copy link

claude bot commented Jan 3, 2026

PR Review: fix(rivetkit): fix c.client not respecting correct origin

Summary

This PR appears to contain formatting-only changes with no functional modifications. The diff shows:

  • Line 168: Reformatted multi-line Object.keys(config.use) to single line
  • Line 438-440: Reformatted createTestWebSocketProxy call to multi-line
  • Line 558: Reformatted modifyManagerRouter call to single line

Issues & Concerns

⚠️ Critical: Title doesn't match changes

  • The PR title claims to fix c.client not respecting correct origin
  • However, no code changes related to c.client or origin handling are present
  • The diff only contains formatting/whitespace changes

Questions

  1. Is this the correct PR? The title suggests there should be functional changes related to client origin handling, but the diff only shows formatting changes.
  2. Was the wrong commit pushed? There may be a mismatch between the intended fix and what was committed.
  3. Should this be closed? According to CLAUDE.md, developers should not run formatting tools manually (cargo fmt is mentioned as something not to run automatically). These formatting changes may be premature.

Recommendations

  1. Verify the PR contents - Ensure the correct changes were pushed
  2. Update the title if this is indeed just a formatting change (e.g., chore(rivetkit): format router.ts)
  3. Close this PR if it was created in error or if formatting should be handled separately per the project's formatting workflow

Notes

  • No functional bugs introduced (since there are no functional changes)
  • No security concerns
  • No performance impact
  • No test coverage changes needed

Verdict: Cannot approve until the mismatch between title and content is resolved. Please clarify the intent of this PR.

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