Skip to content

Conversation

@NathanFlurry
Copy link
Member

Summary

  • simplify manual serverfull connection check to treat any connected runner as success

Testing

  • not run (not requested)

Codex Task

@linear
Copy link

linear bot commented Dec 12, 2025

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 12, 2025 22:43 Inactive
@vercel
Copy link

vercel bot commented Dec 12, 2025

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

Project Deployment Review Updated (UTC)
rivet-inspector Ready Ready Preview, Comment Dec 12, 2025 10:46pm
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 12, 2025 10:46pm
rivet-site Ignored Ignored Preview Dec 12, 2025 10:46pm
rivetkit-serverless Skipped Skipped Dec 12, 2025 10:46pm

@claude
Copy link

claude bot commented Dec 12, 2025

PR Review - Simplify runner connection detection

Summary

This PR simplifies the manual serverfull connection check by removing specific datacenter and runner name validation, instead treating any connected runner as a successful connection.


Analysis

What Changed

Before:

const { watch } = useFormContext();
const datacenter: string = watch("datacenter");
const runnerName: string = watch("runnerName");

const success = !!queryData?.find(
    (runner) =>
        runner.datacenter === datacenter && runner.name === runnerName,
);

After:

const success = !!queryData?.length;

This change removes the specific matching logic that verified the runner's datacenter and name match the form values.


Concerns

🔴 Critical: Behavior Change May Lead to False Positives

The new implementation has a significant semantic change:

  1. Previous Behavior: Only marks connection as successful when a runner with the exact datacenter AND name specified in the form is detected
  2. New Behavior: Marks connection as successful if any runner exists in the namespace, regardless of datacenter or name

Impact: Users could see "Runner successfully connected" even if:

  • The runner they configured hasn't actually connected yet
  • A different runner from a previous setup is still connected
  • The runner is connected to a different datacenter than specified

Example Scenario:

  1. User has an old runner "old-runner" in datacenter "us-east" that's still running
  2. User fills out form to connect "new-runner" in datacenter "eu-west"
  3. User sees success message immediately (because "old-runner" exists)
  4. User proceeds thinking "new-runner" is connected when it isn't

⚠️ Missing Context on Why This Change Was Made

The PR description states:

"simplify manual serverfull connection check to treat any connected runner as success"

However, it's unclear:

  • Why was the specific validation problematic?
  • When is this simplified check more appropriate?
  • Are there use cases where any runner is indeed sufficient?

Questions & Recommendations

1. Is this the intended behavior?

  • Should the connection check verify the specific runner the user configured?
  • Or is the goal to just verify that any runner is available in the namespace?

2. Alternative: Fix the original logic instead?

If there was a bug with the original matching logic (e.g., data format mismatch), consider:

  • Debugging why the original find() wasn't working
  • Checking the structure of data returned by runnersQueryOptions()
  • Ensuring field names match (e.g., datacenter vs region, name vs runnerName)

3. Consider UX implications

If treating any runner as success is intentional:

  • Update the success message to be clearer: "A runner is connected" instead of "Runner successfully connected"
  • Consider adding additional guidance to help users verify the correct runner connected
  • Add logging to help debug if the wrong runner connected

4. Potential Fix (if specific validation is needed):

const { watch } = useFormContext();
const datacenter: string = watch("datacenter");
const runnerName: string = watch("runnerName");

const success = !!queryData?.find(
    (runner) => {
        // Debug: log to see actual structure
        console.log('Runner data:', runner);
        return runner.datacenter === datacenter && runner.name === runnerName;
    }
);

Code Quality Notes

Good:

  • Simplifies code by removing unused hook (useFormContext)
  • Reduces complexity

⚠️ Formatting:

  • Line 68 uses spaces instead of tabs (inconsistent with codebase style)
  • Per CLAUDE.md: "Hard tabs for Rust formatting" - while this is TypeScript/TSX, check if the frontend follows similar conventions

Testing Recommendations

Given this behavior change, please verify:

  1. Fresh namespace: Does the connection check work correctly when no runners exist?
  2. Multiple runners: If multiple runners exist, does the UI correctly indicate which one matched?
  3. Wrong datacenter: If a runner connects to the wrong datacenter, is this surfaced to the user?
  4. Form state: When the form values change, does the success state update appropriately?

Verdict

⚠️ Request Changes - This change appears to introduce a significant behavioral regression unless there's additional context I'm missing. Please clarify:

  1. The reason for removing specific runner validation
  2. Whether false positives (any runner = success) are acceptable
  3. How users will know if the correct runner connected

If this simplified check is intentional for specific use cases, please update the PR description with the rationale and consider UX improvements to make the behavior clear to users.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 12, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 2c1fdce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants