Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Oct 26, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 26, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Ready Ready Preview Comment Oct 27, 2025 11:34pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 27, 2025 11:34pm
rivet-site Ignored Ignored Preview Oct 27, 2025 11:34pm

@jog1t jog1t marked this pull request as ready for review October 26, 2025 22:49
Copy link
Contributor Author

jog1t commented Oct 26, 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.

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 26, 2025

Graphite Automations

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

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

@claude
Copy link

claude bot commented Oct 26, 2025

PR Review: Fix redirect engine users to connect page when there's no actors

Overview

This PR adds the same redirect logic that exists for the cloud route to the engine route, ensuring engine users are also redirected to the connect page when there are no actors (builds or runner configs). Good consistency improvement!

✅ Positives

  1. Code Reuse: Excellent extraction of shouldDisplayActors into a shared export with proper TypeScript typing.
  2. Consistency: Engine route now matches the cloud route's behavior, providing a better UX.
  3. Type Safety: The explicit type annotation on shouldDisplayActors is well-defined and makes the contract clear.
  4. Clean Refactoring: The removal of unnecessary fragment wrapper in the cloud route component improves code clarity.

🔍 Observations & Suggestions

1. Type Definition Location (Minor)

The shouldDisplayActors function is exported from the cloud route file but used by the engine route. While this works, consider:

  • Current approach: Engine imports from cloud route (tight coupling)
  • Alternative: Move to a shared utility file (e.g., /frontend/src/utils/actors.ts or /frontend/src/lib/route-utils.ts)

This would make the dependency relationship clearer and avoid route-to-route imports.

Example:

// frontend/src/lib/route-utils.ts
import type { QueryClient } from "@tanstack/react-query";
import type { useEngineCompatDataProvider } from "@/components/actors";

export async function shouldDisplayActors(context: {
  queryClient: QueryClient;
  dataProvider: ReturnType<typeof useEngineCompatDataProvider>;
}) {
  // ... existing implementation
}

2. Return Value Semantics (Minor)

The function returns true | undefined instead of boolean. While this works with the redirect condition (if (!shouldDisplay)), it's semantically unclear:

if (!hasBuilds && !hasRunnerConfigs) {
  return undefined;  // Why undefined instead of false?
}
return true;

Suggestion: Return false instead of undefined for clarity, or document why undefined is intentional:

// If no builds or configs, don't display actors page
if (!hasBuilds && !hasRunnerConfigs) {
  return false;
}
return true;

3. Error Handling Consistency

The function has a catch-all that returns undefined:

} catch {
  return undefined;
}

This means any error (network failure, permission issue, etc.) will redirect to the connect page. Consider:

  • Is this the desired behavior for all error types?
  • Should certain errors be logged for debugging?
  • Would it be better to let some errors propagate?

Suggestion: Add minimal logging for production debugging:

} catch (error) {
  tracing::warn!(?error, "failed to check if actors should display");
  return undefined;
}

Wait, this is TypeScript, not Rust! 😄

} catch (error) {
  console.warn("Failed to check if actors should display", error);
  return false;
}

4. Missing Test Coverage (Medium Priority)

There don't appear to be any tests for:

  • The shouldDisplayActors function
  • The redirect behavior in beforeLoad
  • Edge cases (empty pages, network errors, etc.)

Suggestion: Add tests for this routing logic to prevent regressions.

5. Fragment Removal (Nitpick)

Nice cleanup removing the unnecessary fragment in frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:73-74! However, the engine route still has a fragment wrapper (line 30-39 in the new version). Consider applying the same cleanup there for consistency.

🐛 Potential Issues

None identified - the code appears functionally correct.

🔒 Security Considerations

No concerns - this is client-side routing logic with no security implications.

⚡ Performance Considerations

The beforeLoad hook will make two API calls (fetchInfiniteQuery for builds and runner configs) on every navigation to these routes. This is acceptable for routing decisions, but be aware:

  • These queries are necessary to determine if redirect is needed
  • The queries should be cached by React Query, minimizing performance impact
  • Consider if the 5000ms refetchInterval in the connect page could conflict with the cache

📝 Summary

Recommendation: Approve with minor suggestions

This is a solid PR that improves consistency between cloud and engine routes. The code is functional and well-structured. The suggestions above are minor improvements for code organization and clarity, not blockers.

Action Items (Optional):

  • Consider moving shouldDisplayActors to a shared utilities file
  • Change undefined return to false for clarity
  • Add test coverage for the redirect logic
  • Apply the fragment cleanup to the engine route as well

Great work! 🚀

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 26, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: fb4f216

@jog1t jog1t requested a review from NathanFlurry October 26, 2025 23:14
@jog1t jog1t force-pushed the 10-26-fix_fe_redirect_engine_users_to_connect_page_when_there_s_no_actors branch from c352b7a to c22f51f Compare October 27, 2025 23:25
@jog1t jog1t force-pushed the 10-26-fix_fe_empty_token_it_is_a_valid_token branch from 31cd0a4 to 03bc5d3 Compare October 27, 2025 23:25
@claude
Copy link

claude bot commented Oct 27, 2025

Code Review - PR #3269

Summary

This PR adds redirect logic for engine users to the connect page when there are no actors, bringing parity with the cloud route behavior.


✅ Positive Aspects

  1. Good code reuse: Extracting shouldDisplayActors as an exported function and reusing it in the engine route is the right approach
  2. Consistent UX: Engine users now get the same redirect behavior as cloud users when there are no builds or runner configs
  3. Type safety improvements: The function signature is more explicit with a well-defined context type instead of using InferAllContext
  4. Minor cleanup: Removing unnecessary React fragment wrapper in the cloud route

🐛 Issues & Concerns

1. Type Safety Concern (Medium Priority)

The shouldDisplayActors function expects a specific context shape with queryClient and dataProvider, but there's no runtime validation that the engine context provides these fields.

Location: frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:18

Issue: If the engine context doesn't have the exact same shape as the cloud context, this will fail at runtime. TypeScript won't catch this because the context type is checked at the route level (context.__type !== "engine"), not at the function level.

Recommendation: Either:

  • Add runtime validation to ensure the context has the required fields
  • Create a shared type/interface that both contexts explicitly implement
  • Add a comment documenting the assumption that both contexts have the same structure

2. Return Type Inconsistency (Low Priority)

The function returns true | undefined, which is semantically confusing.

Location: frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:54-60

if (!hasBuilds && !hasRunnerConfigs) {
  return undefined;  // This seems odd
}

return true;

Issue: The function returns undefined for both error cases (caught exceptions) and the "no builds/configs" case. This makes it hard to distinguish between an error and a legitimate empty state.

Recommendation: Consider returning false instead of undefined for the no-builds case, or using a more explicit return type like boolean | null where null specifically indicates an error state.

3. Missing Error Handling Context (Low Priority)

The catch block silently returns undefined without any logging.

Location: frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:59-61

Issue: If the query fails, there's no indication of what went wrong, making debugging difficult.

Recommendation: Add logging or error tracking:

} catch (error) {
  console.error('Failed to check for actors:', error);
  return undefined;
}

4. Import Organization (Style)

The import is using a relative path that goes up multiple levels.

Location: frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:9

import { shouldDisplayActors } from "../../_cloud/orgs.$organization/projects.$project/ns.$namespace/index";

Issue: This creates coupling between engine and cloud routes. If the cloud route moves, the engine route breaks.

Recommendation: Consider moving shouldDisplayActors to a shared utilities file (e.g., @/utils/actors or @/components/actors/utils) to make the dependency more explicit and maintainable.


🔒 Security Considerations

No security concerns identified. The redirect logic is client-side only and doesn't expose sensitive data.


⚡ Performance Considerations

Potential N+1 Query Issue: The function fetches infinite queries for both builds and runner configs on every route load. This happens in beforeLoad, which runs before rendering.

Impact: Acceptable for now, but consider:

  • Caching these queries if they're also used elsewhere
  • The queries appear to already use React Query, which should handle caching

🧪 Test Coverage

Missing: No tests found for this logic.

Recommendation: Add tests for:

  • Redirect behavior when no builds/configs exist
  • No redirect when builds exist
  • No redirect when runner configs exist
  • Error handling in shouldDisplayActors

📝 Additional Suggestions

  1. Type export: Consider exporting the context type from a shared location:
export type ActorDisplayContext = {
  queryClient: QueryClient;
  dataProvider: ReturnType<typeof useEngineCompatDataProvider>;
};
  1. Documentation: Add JSDoc to explain the redirect behavior:
/**
 * Determines if the actors page should be displayed based on
 * whether there are any builds or runner configs available.
 * Redirects to the connect page if neither exist.
 * 
 * @returns true if actors should be displayed, undefined otherwise
 */
export async function shouldDisplayActors(/* ... */) {

📊 Overall Assessment

Code Quality: ✅ Good
Functionality: ✅ Appears correct
Risk Level: 🟡 Low-Medium (mostly due to type safety assumptions)

This is a solid PR that achieves its goal. The main concerns are around type safety and code organization. Consider addressing the type safety issue before merging.

@claude
Copy link

claude bot commented Oct 27, 2025

Code Review: PR #3269 - Redirect engine users to connect page when there's no actors

Summary

This PR adds logic to redirect users to the connect page when they don't have any builds or runner configs. The implementation shares the shouldDisplayActors function between cloud and engine routes.

Positive Aspects

  1. Good code reuse: Extracting shouldDisplayActors as an exported function and sharing it between cloud and engine routes follows DRY principles
  2. Type safety improvement: The function signature now uses explicit types instead of InferAllContext, making it more portable and clearer
  3. Consistent UX: Engine users now get the same redirect behavior as cloud users when no actors are available
  4. Clean refactoring: Removed unnecessary React Fragment wrapper in the cloud route component

Issues & Concerns

1. Type Safety Concern (High Priority)

The shouldDisplayActors function expects a specific context shape with queryClient and dataProvider, but when called from the engine route, it passes the entire context without type validation.

Location: frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:18

const shouldDisplay = await shouldDisplayActors(context);

The engine context may not have the exact same shape as the cloud context. Looking at the route definitions:

  • Cloud route (ns.$namespace.tsx): Uses beforeLoad which has queryClient in context
  • Engine route (ns.$namespace.tsx): Uses context which transforms the dataProvider

Recommendation: Verify that the engine context at the index route level has access to queryClient and dataProvider with the required methods (buildsQueryOptions, runnerConfigsQueryOptions). Consider adding runtime checks or more explicit type guards.

2. Inconsistent Error Handling (Medium Priority)

The shouldDisplayActors function returns undefined on error (line 60), but the calling code checks for falsy values:

if (!shouldDisplay) {
  throw redirect({ from: Route.to, replace: true, to: "./connect" });
}

This means that if there's an error fetching builds/configs, users will be redirected to the connect page, which may not be the intended behavior. Errors could occur due to network issues, permission problems, etc.

Recommendation: Consider differentiating between "no actors" (should redirect) and "error checking for actors" (should show error or retry). Perhaps:

  • Return false for "no actors found"
  • Return true for "actors exist"
  • Throw or return a specific error value for actual errors

3. Inconsistent Return Values (Low Priority)

The function returns undefined in two cases and true in one case:

if (!hasBuilds && !hasRunnerConfigs) {
  return undefined;  // No actors
}
return true;  // Has actors
// catch block also returns undefined  // Error case

Recommendation: Use boolean values consistently (false instead of undefined) for clarity.

4. Missing CatchBoundary in Engine Route (Low Priority)

The cloud route wraps <Actors> in a CatchBoundary, but the engine route component doesn't have the same error boundary structure after this change.

Location: frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:30-40

The cloud route has:

<CatchBoundary getResetKey={() => actorId ?? "no-actor-id"}>
  <Actors actorId={actorId} />
  <CatchBoundary ...>
    {!n ? <BuildPrefiller /> : null}
  </CatchBoundary>
</CatchBoundary>

But the engine route has:

<>
  <Actors actorId={actorId} />
  <CatchBoundary ...>
    {!n ? <BuildPrefiller /> : null}
  </CatchBoundary>
</>

Recommendation: Add the outer CatchBoundary for consistency and better error handling.

Testing Recommendations

Since there are no test files for these routes, consider manual testing:

  1. ✅ Test engine route with no builds and no runner configs → should redirect to connect
  2. ✅ Test engine route with builds but no runner configs → should show actors
  3. ✅ Test engine route with runner configs but no builds → should show actors
  4. ⚠️ Test engine route when API calls fail (network error) → verify expected behavior
  5. ✅ Test cloud route continues to work as before

Security & Performance

  • ✅ No security concerns identified
  • ✅ Performance is acceptable - queries are fetched once during route loading
  • ✅ Good use of replace: true in redirect to avoid polluting browser history

Style & Convention Notes

According to CLAUDE.md, the codebase uses:

  • ✅ Hard tabs for Rust (N/A for this TypeScript PR)
  • ✅ Follows existing patterns from neighboring files
  • ✅ Proper import organization

Recommendation

Approve with minor changes suggested. The main concern is ensuring type safety for the engine context. I'd recommend:

  1. Add explicit type checking or validation that the engine context has the required shape
  2. Improve error handling to distinguish between "no actors" and "error fetching actors"
  3. Consider adding the outer CatchBoundary to the engine route for consistency

Great work on the code reuse and maintaining consistency between cloud and engine experiences!

Base automatically changed from 10-26-fix_fe_empty_token_it_is_a_valid_token to main October 27, 2025 23:30
@jog1t jog1t force-pushed the 10-26-fix_fe_redirect_engine_users_to_connect_page_when_there_s_no_actors branch from c22f51f to fb4f216 Compare October 27, 2025 23:32
@claude
Copy link

claude bot commented Oct 27, 2025

PR Review: Redirect engine users to connect page when there's no actors

Overview

This PR adds redirect logic to the engine route to match the existing cloud route behavior, ensuring users without actors/builds are redirected to the connect page. Overall the implementation looks good, but I've identified a few areas for improvement.


Code Quality & Best Practices

✅ Positive Points

  1. Good code reuse: Extracting shouldDisplayActors to be shared between cloud and engine routes is the right approach
  2. Consistent behavior: The engine route now matches the cloud route's redirect logic
  3. Clean refactoring: Removal of unnecessary React fragment wrappers improves code clarity

⚠️ Areas for Improvement

1. Type Safety Concern (frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:32-35)

The shouldDisplayActors function signature uses ReturnType<typeof useEngineCompatDataProvider>, but useEngineCompatDataProvider is a React hook that returns runtime values, not a type. This could cause type issues.

Recommendation:

// Instead of using ReturnType on a hook, define an explicit interface
export interface ActorsDisplayContext {
  queryClient: QueryClient;
  dataProvider: {
    buildsQueryOptions: () => any;
    runnerConfigsQueryOptions: () => any;
  };
}

export async function shouldDisplayActors(context: ActorsDisplayContext) {
  // ...
}

2. Inconsistent Return Values (frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:54-61)

The function returns undefined, true, or implicitly undefined (from catch). This is confusing.

Current:

if (!hasBuilds && !hasRunnerConfigs) {
  return undefined;  // Why undefined instead of false?
}
return true;

Recommendation:

// Be explicit and consistent
if (!hasBuilds && !hasRunnerConfigs) {
  return false;
}
return true;

3. Silent Error Handling (frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:59-61)

The catch block silently returns undefined without logging. This could hide real issues.

Recommendation:

} catch (error) {
  // Log the error for debugging
  console.error('Failed to check if actors should be displayed:', error);
  return false;
}

Potential Bugs & Issues

1. Cross-Route Import Path (frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:9)

Importing from a sibling route using a relative path like ../../_cloud/orgs.$organization/projects.$project/ns.$namespace/index is fragile and creates tight coupling between routes.

Recommendation:

  • Move shouldDisplayActors to a shared utility file like @/lib/actors-utils.ts or @/components/actors/utils.ts
  • This improves maintainability and makes the dependency clearer

2. Context Type Compatibility (frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:18)

The engine context is passed to shouldDisplayActors, but we should verify the engine context structure matches the expected type (has queryClient and dataProvider with the required methods).


Performance Considerations

1. Double Query Fetch (frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:37-48)

The beforeLoad function fetches builds and runner configs, and then the component likely fetches them again. Consider if React Query's cache will prevent duplicate network requests, or if there's a way to pass the results forward.


Security Concerns

✅ No security issues identified. The redirect logic doesn't expose sensitive information.


Test Coverage

⚠️ Missing Tests: This PR lacks test coverage for the new redirect logic.

Recommendations:

  1. Add unit tests for shouldDisplayActors covering:
    • Returns true when builds exist
    • Returns true when runner configs exist
    • Returns false when neither exist
    • Returns false on error
  2. Add integration tests for the redirect behavior in both routes

Minor Issues

1. Variable Naming (frontend/src/routes/_context/_cloud/orgs.$organization/projects.$project/ns.$namespace/index.tsx:24)

Good change from isVisible to shouldDisplay - more descriptive! ✅

2. Missing CatchBoundary in Engine Route (frontend/src/routes/_context/_engine/ns.$namespace/index.tsx:30-31)

The engine route is now missing the outer CatchBoundary that wraps the Actors component. This was likely intentional to match the cloud route, but verify this is the desired behavior.


Summary

Severity Breakdown:

  • 🔴 High: 1 (Cross-route import, should be moved to shared location)
  • 🟡 Medium: 2 (Type safety, error handling)
  • 🟢 Low: 2 (Return values, test coverage)

Recommendation: Request changes to address the type safety issue and move the shared function to a proper utility location. The other issues are nice-to-haves but not blockers.


Suggested Action Items

  1. Move shouldDisplayActors to a shared utility file (not a route file)
  2. Define an explicit interface instead of using ReturnType<typeof hook>
  3. Make return values explicit (use false instead of undefined)
  4. Add basic test coverage
  5. Consider logging errors in the catch block

Great work on improving consistency between engine and cloud routes! 🚀

@jog1t jog1t merged commit 0d19306 into main Oct 27, 2025
10 checks passed
@jog1t jog1t deleted the 10-26-fix_fe_redirect_engine_users_to_connect_page_when_there_s_no_actors branch October 27, 2025 23:35
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