fix(fe): show connect page when there are no runner configs but runner names

[jog1t]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-inspector	Ready	Preview	Comment	Nov 4, 2025 8:26pm
rivetkit-serverless	Ready	Preview	Comment	Nov 4, 2025 8:26pm

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 4, 2025 8:26pm
rivet-site	Ignored	Preview		Nov 4, 2025 8:26pm

[jog1t]

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

• fix(fe): show connect page when there are no runner configs but runner names #3298 : 2 dependent PRs (#3306 , #3310 ) 👈 (View in Graphite)
• chore(rivetkit): log version with error logs #3303
• chore(rivetkit): log close code & reason in disconnect warning #3302
• feat(rivetkit): integrate websocket hibernation #3301
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3332 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

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]

PR Review

Summary

This PR fixes a bug where the connect page wasn't displaying properly when there are runner names but no runner configs. The fix changes the conditional logic from checking runnerConfigsCount to checking runnerNamesCount.

Code Quality ✅

Positive Changes:

• Correct semantic fix: The change properly addresses the issue. Runner names (which represent registered runner types) are a better indicator of whether to show the connect UI vs. the onboarding flow, compared to runner configs (which are datacenter-specific configurations).
• Clean refactoring: Removed unused imports (useQuery, useCloudDataProvider, useEngineDataProvider, useRailwayTemplateLink, and z) which improves code cleanliness.
• Consistent naming: Variable names were updated appropriately (runnerConfigsCount → runnerNamesCount, hasConfigs → hasRunnerNames).

Logic Analysis 🔍

Looking at the data provider implementation (engine-data-provider.tsx:460-489), I can confirm:

• runnerNamesQueryOptions() fetches the list of runner names via client.runners.listNames()
• runnerConfigsQueryOptions() fetches datacenter-specific configurations via client.runnerConfigsList()

The fix makes sense because:

• Runner names represent the available runner types that can be used
• Runner configs are datacenter-specific settings for those runners
• A user could have runner names registered but not yet have datacenter configs set up
• The onboarding UI should be skipped if runner names exist, even without configs

Potential Concerns ⚠️

Minor consideration:

• The isLoading variable name is still used but now refers to runner names loading state, not runner configs. This is technically correct but could be slightly more explicit as isLoadingRunnerNames if you want maximum clarity. However, this is a very minor point and the current naming is acceptable.

Performance Considerations ✅

• No performance concerns. The change switches from one API call to another, maintaining the same polling interval (5000ms).
• Both queries use useSuspenseInfiniteQuery, so loading behavior remains consistent.

Security Concerns ✅

• No security issues identified. This is a client-side UI conditional change.

Test Coverage 📝

• Recommendation: Consider adding a test case to verify the UI displays the connect page (not onboarding) when runnerNamesCount > 0 even if runnerConfigsCount === 0. This would prevent regression of this specific bug.

Conclusion

Approved ✅

This is a well-executed bug fix that correctly identifies and resolves the issue. The code is clean, properly handles the unused imports, and the logic makes semantic sense. Great work!

[graphite-app]

Graphite Automations

"Test" took an action on this PR • (10/31/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

[claude]

PR Review

Summary

This PR fixes a bug where the connect page wasn't being displayed when there were runner names registered but no runner configs present. The fix changes the conditional logic to check for runnerNames instead of runnerConfigs.

Code Quality ✅

Positive aspects:

• Correct fix: The change properly addresses the issue described in the title. Checking runnerNamesCount (line 63-65) is the right approach since runner names are registered before configs are created.
• Proper cleanup: Removed unused imports (useQuery, useCloudDataProvider, useEngineDataProvider, useRailwayTemplateLink, z) which improves code cleanliness
• Consistent naming: Variable names were updated logically from runnerConfigsCount → runnerNamesCount and hasConfigs → hasRunnerNames
• Correct data access: The select function properly accesses data.pages[0].names.length which aligns with the runnerNamesQueryOptions() return structure (lines 460-489 in engine-data-provider.tsx)

Potential Issues 🔍

Minor concerns:

1. Data access pattern (Low severity):
   The code only checks the first page: data.pages[0].names.length. If runner names could be paginated and the first page is empty but subsequent pages have data, this would show the connect page incorrectly. However, this is likely not an issue in practice since:

   • The infinite query should return at least one page
   • If you have zero names on the first page, you likely have zero overall
   • The select function in runnerNamesQueryOptions() (line 483) flattens all pages, but this component doesn't use that

2. Refetch interval consistency:
   The 5000ms refetch interval is maintained, which is good for consistency with the Providers and Runners components (lines 361, 407).

Testing Recommendations 🧪

Please verify these scenarios:

1. ✅ No runner names → shows connect page
2. ✅ Runner names exist but no configs → shows connect page
3. ✅ Both runner names and configs exist → shows main page
4. Edge case: Very first runner name being registered (first page empty, but data incoming)

Performance Considerations ⚡

No significant performance concerns. The change actually improves efficiency slightly since listNames is likely faster than runnerConfigsList.

Security 🔒

No security concerns identified. The change is purely frontend logic for display conditions.

Final Verdict ✅

Approved. This is a clean, focused bug fix that correctly addresses the issue. The code is well-structured and the logic is sound. The unused imports cleanup is a nice bonus.

Suggestion: Consider whether using the flattened select result would be more robust:

const { data: runnerNames, isLoading } = useSuspenseInfiniteQuery({
    ...useEngineCompatDataProvider().runnerNamesQueryOptions(),
    refetchInterval: 5000,
});

const hasRunnerNames = runnerNames !== undefined && runnerNames.length > 0;

This would check all pages instead of just pages[0], though the current implementation is likely fine for this use case.

[claude]

PR Review - Code Quality & Best Practices

Summary

This PR fixes a UI state management issue by switching from checking runnerConfigsCount to runnerNamesCount when determining whether to show the connect page. The change is semantically correct and improves the user experience.

---

✅ Positive Aspects

1. Correct semantic fix: The change properly checks for runnerNames instead of runnerConfigs, which aligns with the PR title - showing the connect page when there are no runner configs but there are runner names.

2. Clean import cleanup: Properly removed unused imports (useQuery, z, useRailwayTemplateLink, useCloudDataProvider, useEngineDataProvider) which reduces bundle size and improves code cleanliness.

3. Consistent variable naming: The variable renaming from hasConfigs to hasRunnerNames makes the code more readable and self-documenting.

4. Preserves refetch behavior: The 5-second refetch interval is maintained, ensuring the UI stays updated.

---

🔍 Observations & Considerations

1. Data Structure Differences

The change switches between two different data structures:

• Before: runnerConfigsQueryOptions() → data.pages[0].runnerConfigs (object)
• After: runnerNamesQueryOptions() → data.pages[0].names (array)

While the change looks correct, it's worth noting that runnerNames and runnerConfigs represent different concepts:

• runnerNames: List of runner names that have been registered
• runnerConfigs: Configuration objects for runners

This suggests the original logic may have been checking the wrong condition. ✅ This is the core fix.

2. Potential Edge Case

Consider the scenario:

• Runner names exist (runnerNamesCount > 0)
• But no runner configs exist (runnerConfigsCount === 0)

In this case, the new code will show the main connect page (with Providers and Runners tables), but the Providers component below (line 352-396) still queries runnerConfigsQueryOptions(), which could show an empty state. This is likely the correct behavior, but worth documenting or testing.

3. Loading State

The isLoading variable from useSuspenseInfiniteQuery should theoretically never be true when using suspense queries (suspense handles loading states). The loading check on line 72 might be redundant, but keeping it doesn't hurt and provides defensive programming.

4. Performance Consideration

The query now fetches runnerNames which is likely lighter than fetching full runnerConfigs. This is a positive performance improvement. ✅

---

🧪 Testing Recommendations

While I don't see test files for this component, consider manually testing these scenarios:

1. ✅ Empty state: No runner names, no configs → Should show connect/setup page
2. ✅ Names but no configs: Runner names exist, no configs → Should show main page with empty configs table
3. ✅ Both exist: Both names and configs exist → Should show main page with populated tables
4. ✅ Network error handling: Ensure error states are properly handled by the query

---

📝 Minor Suggestions

1. Type safety: Consider adding a comment explaining why we check runnerNamesCount instead of checking configs, to prevent future confusion:

   // Check for runner names instead of configs since runners may exist without configs
   const hasRunnerNames = runnerNamesCount !== undefined && runnerNamesCount > 0;

2. Consistent pattern: The Providers and Runners components below use useInfiniteQuery while the main check uses useSuspenseInfiniteQuery. This is fine, but ensure it's intentional (suspense for critical path, non-suspense for sections).

---

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Performance improved (lighter query)
• ✅ No unnecessary re-renders introduced
• ✅ Proper cleanup of unused code

---

Final Verdict

Approve ✅

This is a clean, focused fix that addresses a specific UI state management issue. The code quality is good, imports are properly cleaned up, and the logic is sound. The change is minimal and low-risk.

Recommendation: Merge after manual testing of the edge cases mentioned above.

[pkg-pr-new]

More templates

• example-ai-agent
• example-background-jobs
• example-better-auth-external-db
• example-bots
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-counter
• example-counter-serverless
• example-crdt
• example-cursors
• example-cursors-raw-websocket
• example-database
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-freestyle
• example-game
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• example-smoke-test
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc
• example-workflows

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: d5c5825

[claude]

Code Review

Summary

This PR fixes a bug where the connect page was incorrectly checking for runner configs instead of runner names to determine whether to show the onboarding flow. This is a good fix that addresses the issue described in the title.

Positive Changes ✅

1. Correct Logic: The change from runnerConfigsQueryOptions() to runnerNamesQueryOptions() is semantically correct. Runner names represent actual connected runners, while runner configs are just configurations. Checking for active runners is the right approach for determining whether to show the connect/onboarding page.

2. Cleanup: The PR properly removes unused imports (useQuery, useCloudDataProvider, useEngineDataProvider, useRailwayTemplateLink, and z), keeping the codebase clean.

3. Naming: Variable names have been appropriately updated (runnerConfigsCount → runnerNamesCount, hasConfigs → hasRunnerNames) for clarity.

4. API Consistency: The data shape is consistent - both queries return data with pagination, and the selection logic (data.pages[0].names.length vs Object.values(data.pages[0].runnerConfigs).length) is appropriate for each API.

Observations & Suggestions 💡

1. Loading State Issue: The component has a logic issue (pre-existing, not introduced by this PR):

   const { data: runnerNamesCount, isLoading } = useSuspenseInfiniteQuery({
       ...
   });
   
   if (isLoading) {
       return <LoadingUI />;
   }

   Since this uses useSuspenseInfiniteQuery, isLoading should never be true at this point - the suspense boundary would handle the loading state. The if (isLoading) block on lines 72-112 is likely dead code and could be removed in a future cleanup.

2. Data Access Pattern: The code accesses data.pages[0].names.length directly. Consider adding optional chaining for safety:

   select: (data) => data.pages[0]?.names?.length ?? 0,

   Though with useSuspenseInfiniteQuery, this should be safe.

3. Refetch Interval: The 5-second polling (refetchInterval: 5000) is appropriate for detecting when runners connect, but consider whether this could be optimized with WebSocket updates in the future for better real-time feedback.

4. Test Coverage: No tests found for this component. Consider adding tests that verify:

   • Onboarding page shows when runnerNamesCount === 0
   • Main connect page shows when runnerNamesCount > 0
   • Loading state renders correctly

Security & Performance 🔒

• No security concerns identified
• Performance impact is minimal - the API call is likely simpler/faster than fetching full configs
• Polling interval is reasonable

Verdict

Approved ✅ - This is a straightforward bug fix that correctly addresses the issue. The logic change makes sense and the implementation is clean.

Minor Suggestion

Consider extracting the runner check logic into a custom hook for reusability:

const useHasConnectedRunners = () => {
  const { data: runnerNamesCount } = useSuspenseInfiniteQuery({
    ...useEngineCompatDataProvider().runnerNamesQueryOptions(),
    select: (data) => data.pages[0].names.length,
    refetchInterval: 5000,
  });
  
  return runnerNamesCount > 0;
};

But this is optional and the current implementation is fine.

---

Review generated by Claude Code

[claude]

Pull Request Review

Summary

This PR fixes a UX issue where the connect page wasn't showing when there are runner names but no runner configs. The fix correctly changes the conditional logic to check for runner names instead of runner configs.

Analysis

The Fix is Correct:

The change from checking runnerConfigsQueryOptions() to runnerNamesQueryOptions() is the right approach because:

1. Runner names represent actively connected runners - These are actual server instances that have connected to Pegboard and are available to run actors
2. Runner configs represent configurations - These are stored configurations for serverless endpoints (like Vercel, Railway) that may or may not have spawned runners yet

Why the old code was buggy:

The original code could incorrectly show the getting started page in this scenario:

• A developer runs a serverfull (normal) runner locally that connects with a name
• No runner config exists (serverfull runners don't require configs)
• The page would show Create New Project instead of the actual connect interface
• This is confusing because the user already has runners connected!

Code Quality

Strengths:

• Correct logic fix for the UX issue
• Properly removes unused imports (useQuery, z, unused providers/hooks)
• Maintains consistent naming (hasRunnerNames vs hasConfigs)
• Keeps the 5-second refetch interval for real-time updates

Minor Observations:

1. Variable naming (connect.tsx:63-66): The variable is named runnerNamesCount but it is actually the full query result object. The data property contains the count. However, this pattern is consistent with the existing codebase style, so it is acceptable.

2. Only checking first page (connect.tsx:65): This only checks the first page of results via data.pages[0].names.length. If pagination exists and the first page is empty but subsequent pages have data, this would incorrectly show the getting started page. However, this is likely not a practical concern since if there are any runners, they will appear on the first page.

Testing Recommendations

Consider testing these scenarios:

1. No runners at all - Should show getting started page
2. Serverfull runner connected (has name, no config) - Should show connect page
3. Serverless config added (has config, no runners yet) - Should show getting started page (current behavior after fix)
4. Both present - Should show connect page

Scenario 3 might need verification - when a user adds a serverless config (Vercel, Railway), should they still see the getting started page until runners spawn? Or should they see the connect page immediately? The current fix would show the getting started page until the first runner spawns.

Security and Performance

• No security concerns
• Performance is maintained (5s polling is reasonable for this use case)
• No new dependencies added
• Properly uses useSuspenseInfiniteQuery for loading states

Conclusion

Recommendation: APPROVE with minor suggestion to verify scenario 3 behavior

This is a solid bug fix that correctly addresses the UX issue. The code is clean, removes unnecessary imports, and follows existing patterns in the codebase. The only consideration is whether the UX should show the connect page immediately when a serverless config is added (before runners spawn), but that may be the correct behavior since there is nothing to connect until runners are actually running.

Great work on identifying and fixing this edge case!

[claude]

PR Review

Summary

This PR fixes a bug where the connect page wouldn't show when there are runner names but no runner configs. The change switches from checking runnerConfigsCount to checking runnerNamesCount to determine whether to display the connect page.

Code Quality ✅

Positive aspects:

• Clean refactor that removes unused imports (useQuery, z, useRailwayTemplateLink, useCloudDataProvider, useEngineDataProvider)
• Consistent variable naming (runnerNamesCount and hasRunnerNames)
• The fix correctly addresses the logic issue: checking for runner names is more appropriate than checking for runner configs

Potential Issues ⚠️

1. Data Structure Access (Minor)
In line 65, the code accesses data.pages[0].names.length:

select: (data) => data.pages[0].names.length,

Concern: This could throw an error if pages[0] is undefined or if there are no pages returned. However, looking at the query configuration:

• useSuspenseInfiniteQuery is used, which guarantees data is present before rendering
• The runnerNamesQueryOptions always returns at least one page
• There's an isLoading check that guards against this scenario

Recommendation: Consider making this more defensive:

select: (data) => data.pages[0]?.names?.length ?? 0,

2. Semantic Correctness (Question)
The change checks for runnerNames (registered runner configurations that can be deployed) instead of runnerConfigs (cloud provider configurations).

Question: Is this the correct semantic check for showing the connect page? Consider these scenarios:

• Scenario A: User has runner names but no runner configs → Shows connect page ✅ (this PR's behavior)
• Scenario B: User has runner configs but no runner names → Shows connect page ❌ (would show the full page)

Looking at line 360-362, the Providers component still uses runnerConfigsQueryOptions(), so that section will display configs when available. The title suggests this change is intentional and correct for the use case.

Performance Considerations ✅

Positive:

• The refetchInterval: 5000 is appropriate for this use case
• Using useSuspenseInfiniteQuery is correct for this loading pattern
• The query only fetches runner names (lighter than full configs)

Security Concerns ✅

No security concerns identified. The change is purely UI logic.

Test Coverage ⚠️

Missing:

• No tests included in this PR
• Consider adding integration tests for:
  1. Connect page shows when runnerNames exist but runnerConfigs don't
  2. Connect page shows when neither exist
  3. Full page shows when runnerNames exist

Suggestions

Documentation:
The PR body is empty. Consider adding:

• Brief description of the bug being fixed
• Steps to reproduce the original issue
• Expected vs actual behavior

Code improvement (optional):

const { data: runnerNamesCount, isLoading } = useSuspenseInfiniteQuery({
  ...useEngineCompatDataProvider().runnerNamesQueryOptions(),
  select: (data) => data.pages[0]?.names?.length ?? 0,
  refetchInterval: 5000,
});

Verdict: ✅ Approve with minor suggestions

This is a clean, focused bug fix that correctly addresses the issue. The logic change makes semantic sense, and the cleanup of unused imports is appreciated. The only minor concern is the defensive data access, but the existing guards make this safe in practice.

[claude]

Code Review

Summary

This PR fixes a logic issue where the connect page wasn't displaying correctly when runner names exist but no runner configs are present. The change correctly switches from checking runner configs to checking runner names.

---

Positive Aspects ✅

1. Correct Logic Fix: The change properly addresses the issue described in the PR title. Checking runnerNames instead of runnerConfigs makes semantic sense - the page should show when actual runners are connected, regardless of configuration state.

2. Clean Refactoring: Good cleanup of unused imports (useQuery, useCloudDataProvider, useEngineDataProvider, useRailwayTemplateLink, and z).

3. Data Structure Alignment:

   • runnerNames returns data.pages[0].names.length (array length)
   • runnerConfigs returns Object.values(data.pages[0].runnerConfigs).length (object values count)
   • The new approach is simpler and more direct.

---

Concerns & Suggestions

1. Variable Naming Consistency 🔍

The variable is named runnerNamesCount, but the select function returns the actual length, not the data:

select: (data) => data.pages[0].names.length,

This means runnerNamesCount is actually a number, not a count object. Consider renaming for clarity:

const { data: runnerNamesLength, isLoading } = useSuspenseInfiniteQuery({
  ...useEngineCompatDataProvider().runnerNamesQueryOptions(),
  select: (data) => data.pages[0].names.length,
  refetchInterval: 5000,
});

const hasRunnerNames = runnerNamesLength !== undefined && runnerNamesLength > 0;

2. Potential Data Accuracy Issue ⚠️

The select function only checks pages[0].names.length. For paginated data, this could miss runners on subsequent pages. If a user has 0 runners on page 1 but runners on page 2+, the check would incorrectly show hasRunnerNames = false.

Suggested fix:

select: (data) => data.pages.flatMap((page) => page.names).length,

This would count all runner names across all pages, which aligns with the existing pattern used in the runnerNamesQueryOptions definition (line 483 in engine-data-provider.tsx).

3. Loading State Logic 🤔

There's a potential issue with the loading check:

if (isLoading) {
  return <LoadingUI />;
}

if (!hasRunnerNames) {
  return <EmptyStateUI />;
}

Since you're using useSuspenseInfiniteQuery, isLoading might always be false because suspense queries suspend instead of returning a loading state. The loading skeleton might never show. Consider:

• Verifying if this loading check is necessary with suspense queries
• Or using the route's pendingComponent for loading states (which is already defined)

4. Missing Test Coverage 📝

No tests were found for this component. Given this is a critical user flow (showing the connect page), consider adding tests for:

• Shows empty state when no runner names exist
• Shows connect UI when runner names exist
• Handles loading states correctly
• Refetch interval works as expected

5. Type Safety 💡

The check runnerNamesCount !== undefined && runnerNamesCount > 0 is defensive, but with the select function always returning a number, you could simplify to:

const hasRunnerNames = runnerNamesCount > 0;

Unless there's a specific edge case where select could return undefined.

---

Security & Performance

• ✅ No security concerns identified
• ✅ Refetch interval (5000ms) is reasonable for this use case
• ✅ Query is properly configured with error handling via the data provider

---

Recommendation

The core fix is correct and addresses the issue, but I recommend:

1. Medium priority: Fix the pagination issue in the select function to count all pages
2. Low priority: Consider renaming runnerNamesCount to better reflect it's a length/number
3. Low priority: Review if the isLoading check is necessary with suspense queries

Overall: Approve with suggestions 👍

[claude]

Code Review

Summary

This PR fixes a bug where the connect page was incorrectly determining when to show the onboarding UI. The fix changes from checking runnerConfigsCount to runnerNamesCount, which correctly reflects whether runners are connected.

Positive Aspects ✅

1. Correct Logic Fix: The change properly distinguishes between runner configurations (provider setups) and actual runner instances (running processes). This makes sense because:

   • Runner configs define how/where to run actors (e.g., Vercel, Railway, AWS configs)
   • Runner names represent actual running processes
   • The connect page should be shown when there are no active runners, regardless of config existence

2. Clean Code Cleanup: Properly removed unused imports:

   • useQuery - no longer needed
   • useCloudDataProvider, useEngineDataProvider - unused after refactor
   • useRailwayTemplateLink - removed
   • z (zod) - removed

3. Consistent Naming: Variable names updated to match the new query (runnerNamesCount and hasRunnerNames) which improves code readability

4. Preserves Functionality: The refetch interval (5000ms) and loading states are maintained correctly

Potential Concerns ⚠️

1. Query Selection Logic:

   select: (data) => data.pages[0].names.length

   This only looks at the first page of results. If there are more runner names than fit in one page, they won't be counted. Consider:

   select: (data) => data.pages.flatMap((page) => page.names).length

   However, since this is checking for > 0, the current implementation might be acceptable as a performance optimization.

2. Loading State Consideration: The isLoading variable is extracted from useSuspenseInfiniteQuery but with Suspense queries, isLoading is typically always false after the initial load (since Suspense handles the loading state). The loading check at line 72-112 might be dead code. Consider removing it or verifying it's actually needed.

3. Type Safety: The comparison runnerNamesCount !== undefined && runnerNamesCount > 0 is safe, but could be simplified to just runnerNamesCount > 0 since the length will always be a number (at minimum 0).

Performance Considerations 🚀

• The change from runnerConfigsQueryOptions to runnerNamesQueryOptions is actually better for performance as /runners/names is designed to be more lightweight than fetching full runner configs
• The 5-second refetch interval is reasonable for this use case

Security Concerns 🔒

No security issues identified. Both queries require proper authentication (same namespace context).

Test Coverage 📋

Missing: No tests found for this component. Consider adding tests for:

• Rendering when runnerNamesCount === 0
• Rendering when runnerNamesCount > 0
• Loading state behavior
• Refetch interval functionality

Recommendations

1. Consider flattening all pages in the select function if the count should include all runners across pages
2. Evaluate if the loading state check is necessary with Suspense queries
3. Add test coverage for this critical routing logic
4. Verify edge cases: What happens when the query fails? The throwOnError: noThrow in the data provider suggests errors are swallowed - ensure this is the desired behavior

Verdict

LGTM with minor suggestions 👍

The core fix is sound and addresses the bug correctly. The change makes semantic sense (checking for running processes rather than config existence). The suggestions above are optimizations and quality improvements rather than blockers.

[graphite-app]

Merge activity

• Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
• Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
• Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
• Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

[claude]

Code Review

Summary

This PR fixes a bug where the connect page was not showing when there are runner names but no runner configs. The fix changes the logic to check for runnerNames instead of runnerConfigs to determine whether to show the connect page.

Positive Feedback ✅

• Correct bug fix: The change addresses the root issue by checking for runner names (which represent active runners) instead of runner configs (which are provider configurations). This makes logical sense since runner names indicate actual connected runners.
• Clean refactoring: Removed unused imports (useQuery, useCloudDataProvider, useEngineDataProvider, useRailwayTemplateLink, z) which improves code cleanliness.
• Consistent naming: Variable names changed from runnerConfigsCount/hasConfigs to runnerNamesCount/hasRunnerNames, improving code clarity.
• Maintains existing behavior: The refetch interval (5000ms) and overall page structure remain unchanged.

Code Quality Observations 📋

Good practices:

• Using useSuspenseInfiniteQuery appropriately for data fetching
• Proper use of the select function to transform query data
• Following the existing codebase patterns

Potential improvements (non-blocking):

1. Loading state check redundancy (line 72)

   • The isLoading check may be unnecessary since useSuspenseInfiniteQuery suspends during loading. The suspense boundary should handle the loading state via pendingComponent: DataLoadingPlaceholder.
   • This is likely dead code but not harmful.

2. Type safety consideration:

   • The select function assumes data.pages[0].names exists. While useSuspenseInfiniteQuery guarantees data presence, you might want to add a fallback:

   select: (data) => data.pages[0]?.names?.length ?? 0

3. Semantic naming clarity:

   • hasRunnerNames could be more descriptive like hasConnectedRunners to make the intent clearer to future developers.

Potential Issues 🔍

None identified - The change is safe and addresses the stated problem correctly.

Performance Considerations ⚡

• No performance impact: The change from runnerConfigsQueryOptions to runnerNamesQueryOptions maintains similar query patterns with the same refetch interval.
• Both queries use infinite query with pagination, so performance characteristics should be comparable.

Security Concerns 🔒

None identified - No security implications from this change.

Test Coverage 🧪

Recommendation: Consider adding or verifying the following test scenarios:

1. Page displays connect instructions when no runner names exist (empty state)
2. Page displays main connect UI when runner names exist
3. Transition behavior when first runner connects (runner name appears)
4. Edge case: Runner names exist but runner configs don't (the specific bug this fixes)

Additional Notes

The PR title mentions "when there are no runner configs but runner names" - this fix correctly prioritizes runner names as the source of truth for determining if runners are connected. Runner configs represent cloud provider configurations, while runner names represent actual active runner processes, so this is the correct logic.

Verdict

LGTM! ✅ This is a solid bug fix with good refactoring. The code quality is good and follows repository conventions. The suggested improvements above are optional refinements, not blockers.

[claude]

PR Review: Fix FE Connect Page Display Logic

Summary

This PR fixes a bug where the connect page wasn't showing when there were runner names registered but no runner configs. The fix correctly switches from checking runnerConfigsQueryOptions to runnerNamesQueryOptions.

✅ Positive Findings

1. Correct Fix for the Bug
The change from checking runnerConfigsCount to runnerNamesCount is the right approach. Looking at the data provider code:

• runnerNamesQueryOptions() (line 460-489) returns data.pages[0].names.length - representing runner names
• runnerConfigsQueryOptions() (line 582-622) returns runner configurations

This fix ensures the connect page is displayed when runner names exist, which is the correct condition.

2. Proper Cleanup
The PR removes unused imports that became unnecessary after the refactor:

• Removed unused useQuery import
• Removed unused useCloudDataProvider, useEngineDataProvider
• Removed unused useRailwayTemplateLink and z (zod)

3. Variable Naming
The rename from hasConfigs to hasRunnerNames improves code clarity and accurately reflects what's being checked.

📝 Observations

1. Loading State Behavior
The loading condition on line 72 checks isLoading, but with useSuspenseInfiniteQuery, this should typically never be true since suspense queries suspend during loading. This might be dead code, though it doesn't hurt to keep it for safety.

2. Data Selection
Line 65 uses:

select: (data) => data.pages[0].names.length,

This only checks the first page. If runner names could span multiple pages, this might miss names on subsequent pages. However, for this use case (just checking if any exist), checking the first page is likely sufficient and more performant.

3. Potential Edge Case
The condition on line 69-70:

const hasRunnerNames =
    runnerNamesCount !== undefined && runnerNamesCount > 0;

With useSuspenseInfiniteQuery, runnerNamesCount should never be undefined since the component suspends until data loads. The undefined check might be defensive but unnecessary.

🎯 Code Quality

Style Compliance:

• ✅ No formatting issues
• ✅ Proper TypeScript usage
• ✅ Follows existing patterns in the file
• ✅ Clean import organization

Best Practices:

• ✅ Meaningful variable names
• ✅ Removes dead code
• ✅ Simple, focused change

🔒 Security & Performance

Security: No concerns - this is a frontend display logic change with no security implications.

Performance:

• ✅ The change is actually a performance improvement since runnerNamesQueryOptions fetches less data than runnerConfigsQueryOptions
• ✅ Proper use of select to extract only needed data
• ✅ Reasonable refetchInterval of 5000ms

🧪 Testing

Considerations:

• The fix addresses the specific bug where runner names exist but configs don't
• Manual testing recommended: verify the connect page displays correctly in these states:
  1. No runner names and no configs (should show connect page)
  2. Runner names exist but no configs (should show main page) ← This is the fixed case
  3. Both runner names and configs exist (should show main page)

📊 Overall Assessment

Rating: ✅ Approved

This is a clean, focused bug fix that:

• Correctly addresses the reported issue
• Improves code clarity with better naming
• Removes unused imports
• Has no negative side effects
• Follows project conventions

Minor Suggestions (Optional)

If you want to make the code even more explicit, you could simplify the loading check since useSuspenseInfiniteQuery handles suspension:

const { data: runnerNamesCount } = useSuspenseInfiniteQuery({
    ...useEngineCompatDataProvider().runnerNamesQueryOptions(),
    select: (data) => data.pages[0].names.length,
    refetchInterval: 5000,
});

const hasRunnerNames = runnerNamesCount > 0;

// Remove the isLoading check and skeleton entirely

However, keeping the defensive checks doesn't hurt, so this is purely optional.

---

Recommendation: ✅ Approve and merge - This is a solid bug fix with no issues.