Skip to content

Conversation

@NathanFlurry
Copy link
Member

Summary

  • query runner configs alongside runner connections on the connect page
  • show loading skeleton until both runner names and runner configs resolve
  • hide the quickstart UI when either runners are connected or configs exist
  • isolate runner count queries under distinct keys to avoid clobbering provider data

Testing

  • pnpm biome check frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/connect.tsx

Codex Task

@linear
Copy link

linear bot commented Dec 12, 2025

@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

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

claude bot commented Dec 12, 2025

Code Review

Overview

This PR improves the connect page UX by:

  • Querying both runner connections and runner configs
  • Showing loading skeletons until both queries resolve
  • Hiding the quickstart UI when either runners are connected OR configs exist
  • Isolating runner count queries under distinct keys to prevent data clobbering

✅ Strengths

  1. Good Query Key Isolation (lines 72, 80): The addition of distinct query keys ([...queryKey, "count"]) prevents cache collisions between the count queries and the main queries used elsewhere in the component tree. This is a solid fix for the data clobbering issue mentioned in the PR description.

  2. Improved Loading State: The loading logic now correctly waits for both queries (line 86), ensuring users see the skeleton until all data is available.

  3. Better Conditional Logic (line 129): The condition !hasRunnerNames && !hasRunnerConfigs properly implements the "hide quickstart when either exists" behavior described in the PR.

  4. Clean Refactoring: The removal of unused imports (Link, useParams) keeps the code clean.

🔍 Potential Issues

1. useSuspenseInfiniteQuery Behavior (lines 69-84)

The component uses useSuspenseInfiniteQuery, which should suspend during initial loading. However, the code checks isLoading states and renders a manual loading skeleton (lines 92-127). This seems contradictory:

  • Issue: useSuspenseInfiniteQuery will suspend the component boundary during initial load, so isLoading should never be true in the render function. The manual loading skeleton at lines 92-127 may never be shown.
  • Expected behavior: The DataLoadingPlaceholder (line 58) should handle the suspense boundary loading state.
  • Recommendation: Either:
    • Remove the manual loading check (lines 92-127) and rely on the suspense boundary, OR
    • Switch to regular useInfiniteQuery if you need manual loading state control

2. Inconsistent Loading Patterns

The Providers and Runners components (lines 355-435) use regular useInfiniteQuery (not suspense), while the main component uses suspense queries. This creates inconsistent loading behavior:

  • Main component: Suspense-based loading
  • Child components: Manual loading states

Recommendation: Use consistent query patterns throughout, preferably suspense-based for simpler code.

3. Potential Race Condition

Lines 73-74 and 81-82 both have refetchInterval: 5000, meaning every 5 seconds:

  • If one query completes before the other, users might briefly see the loading skeleton
  • The isLoading check on line 86 could cause flickering between loaded and loading states

Recommendation: Consider using refetchOnWindowFocus: false or notifyOnChangeProps: ['data'] to prevent unnecessary loading state changes during background refetches.

4. Data Access Safety (lines 82, 88, 90)

The code accesses data.pages[0] and counts directly without null checks:

  • Line 82: data.pages[0].runnerConfigs - assumes pages[0] exists
  • While useSuspenseInfiniteQuery guarantees data exists, it doesn't guarantee pages[0] exists if the query returns an empty array

Recommendation: Add safety checks:

select: (data) => data.pages[0]?.names?.length ?? 0
select: (data) => Object.entries(data.pages[0]?.runnerConfigs ?? {}).length

5. Missing Error Handling

The component doesn't handle error states for the suspense queries. If either query fails, the error boundary will catch it, but there's no specific error UI for this page.

Recommendation: Add an error boundary or error handling for a better UX.

🎯 Minor Suggestions

  1. Variable Naming: Consider renaming runnerNamesCount to activeRunnersCount for clarity about what it represents.

  2. Memoization: The query options could be extracted to avoid recreating them on every render:

const runnerNamesQueryOptions = useMemo(
  () => engineCompatDataProvider.runnerNamesQueryOptions(),
  [engineCompatDataProvider]
);
  1. Type Safety: Consider adding explicit types for the query data to catch any structural changes early.

📊 Test Coverage

The PR description mentions running biome check, but there's no mention of:

  • Unit/integration tests for the new loading logic
  • Visual regression tests for the skeleton states
  • Tests for the conditional rendering logic

Recommendation: Add tests to verify:

  • Quickstart shows when both counts are 0
  • Quickstart hides when runnerNames > 0
  • Quickstart hides when runnerConfigs > 0
  • Loading skeleton displays correctly

🔒 Security & Performance

No security concerns identified. Performance considerations:

  • The 5-second polling interval is reasonable for this use case
  • Query key isolation prevents unnecessary cache invalidation ✅

Summary

The core logic is sound, but the mixing of suspense and non-suspense patterns creates confusion. The main concern is whether the manual loading skeleton (lines 92-127) is ever actually rendered given the use of useSuspenseInfiniteQuery. I'd recommend either fully committing to suspense boundaries or switching to regular queries with manual loading states.

@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@3647

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: ad14496

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