refactor: performance improvements for inspector #3452

jog1t · 2025-11-12T19:57:20Z

No description provided.

vercel · 2025-11-12T19:57:25Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-inspector	Error			Nov 12, 2025 7:58pm
rivetkit-serverless	Error			Nov 12, 2025 7:58pm

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 12, 2025 7:58pm
rivet-site	Ignored	Preview		Nov 12, 2025 7:58pm

jog1t · 2025-11-12T19:57:50Z

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 · 2025-11-12T19:59:18Z

Code Review - PR #3452: Performance Improvements for Inspector

Overview

This PR is a major refactor focusing on performance improvements for the inspector. The changes involve removing 129 files (9,810 deletions) and adding 988 lines, resulting in a significant simplification and modernization of the frontend codebase.

Summary

Strengths:

✅ Major cleanup: Removes a large amount of legacy code (129 files deleted)
✅ Modernization: Migrates from Jotai atom-based state to React Query data providers
✅ Better separation of concerns with dedicated data provider pattern
✅ Consistent query/mutation patterns with TanStack Query

Areas for Attention:

⚠️ Performance implications of query options need verification
⚠️ Missing type safety in some areas
⚠️ Potential API call inefficiencies
⚠️ Incomplete error handling

Detailed Findings

1. Code Quality & Architecture

✅ Good: Data Provider Pattern

The new data provider architecture in default-data-provider.tsx and engine-data-provider.tsx is well-structured:

Clear separation between default (stub) and engine implementations
Consistent query/mutation options factory pattern
Type-safe return types

⚠️ Concern: Query Key Type Safety

Location: frontend/src/app/data-providers/engine-data-provider.tsx:65

queryKey: ["namespaces"] as any,

Using as any defeats TypeScript's type checking. Should use proper QueryKey typing:

queryKey: ["namespaces"] as QueryKey,

2. Performance Considerations

⚠️ Potential Issue: Refetch Intervals

Location: frontend/src/app/data-providers/default-data-provider.tsx:57, 109

refetchInterval: 2000,  // Line 57
refetchInterval: 5000,  // Line 109

Concern: Different refetch intervals (2s vs 5s) for similar data could cause performance issues:

The actorsQueryOptions uses 2s refetch
The actorsListQueryOptions uses 5s refetch
Both query the same underlying data

Recommendation:

Document why different intervals are needed
Consider using a WebSocket or Server-Sent Events for real-time updates instead of aggressive polling
Make refetch intervals configurable or conditional based on user activity

⚠️ Potential Issue: Query Invalidation Predicate

Location: frontend/src/app/data-providers/default-data-provider.tsx:216-220

queryClient.invalidateQueries({
  predicate: (query) => {
    return keys.every((k) => query.queryKey.includes(k));
  },
});

Concern: This invalidation pattern may be overly broad and invalidate unrelated queries if key naming isn't carefully managed.

Recommendation: Use more specific query key matching or structured query keys.

3. Potential Bugs

🐛 Bug: Empty Query Always Returns Empty

Location: frontend/src/app/data-providers/engine-data-provider.tsx:230-244

if (
  (opts?.n?.length === 0 || \!opts?.n) &&
  (opts?.filters?.id?.value?.length === 0 ||
    \!opts?.filters?.id?.value ||
    opts?.filters.key?.value?.length === 0 ||
    \!opts?.filters.key?.value)
) {
  return {
    actors: [],
    pagination: { cursor: undefined },
  };
}

Issue: This logic seems incorrect. The condition returns empty results when:

No names AND (no ID filter OR no key filter)

This means if you provide an ID filter but no key filter, it still returns empty. This is likely a logic error. Should probably be:

if (
  (\!opts?.n || opts.n.length === 0) &&
  (\!opts?.filters?.id?.value || opts.filters.id.value.length === 0) &&
  (\!opts?.filters.key?.value || opts.filters.key.value.length === 0)
) {
  // Return empty only if ALL are empty

⚠️ Concern: Actor Transformation Missing Null Checks

Location: frontend/src/app/data-providers/engine-data-provider.tsx:636-660

The transformActor function assumes certain fields exist. Consider adding runtime validation or using optional chaining more defensively.

4. Missing Error Handling

⚠️ Concern: Generic Error Messages

Location: frontend/src/app/data-providers/default-data-provider.tsx:59, 74, 187, etc.

throw new Error("Not implemented");

While these are stub implementations, ensure that the actual implementations have proper error handling with descriptive messages.

⚠️ Concern: Silent Failures

Location: frontend/src/app/data-providers/engine-data-provider.tsx:92, 93, 151, etc.

retry: shouldRetryAllExpect403,
throwOnError: noThrow,

Using noThrow might hide errors from users. Ensure there's proper error state UI to display these to users.

5. Code Style & Best Practices

✅ Good: Consistent Patterns

Query options follow a consistent naming convention (*QueryOptions)
Mutation options follow the same pattern (*MutationOptions)
Good use of TypeScript for type safety (except where noted)

⚠️ Minor: Commented Code

Location: frontend/src/components/actors/actors-actor-details.tsx:150-170, 225-234

Several tabs are commented out (logs, metrics). Consider:

Removing if permanently disabled
Adding TODO comments explaining when they'll be re-enabled
Using feature flags if conditional

6. Testing Considerations

Concern: This is a massive refactor (129 files removed, significant architectural changes), but there's no mention of:

Test coverage for the new data providers
Integration tests for query/mutation flows
Migration testing to ensure no regressions

Recommendation:

Add unit tests for the data provider query/mutation options
Add integration tests for critical user flows
Manual testing checklist for regression testing

7. Performance Validation

Questions to verify:

Has the bundle size changed significantly?
What's the impact on initial load time?
How many API calls are made on initial page load now vs before?
Are there any query waterfalls introduced?

Recommendation: Run performance profiling before/after to validate improvements.

Security Concerns

✅ Good: Auth Handling

The mightRequireAuth metadata pattern is good for tracking which queries need authentication.

⚠️ Note: Header Manipulation

Location: frontend/src/app/data-providers/engine-data-provider.tsx:44-48

Object.keys(args.headers || {}).forEach((key) => {
  if (key.toLowerCase().startsWith("x-fern-")) {
    delete args.headers?.[key];
  }
});

Ensure this header removal is intentional and documented. If Fern headers are being stripped for security/privacy, add a comment explaining why.

Recommendations

High Priority

Fix the query filter logic at engine-data-provider.tsx:230-244
Replace as any with proper typing at engine-data-provider.tsx:65
Add error handling UI for queries with noThrow
Document refetch interval strategy and consider alternatives to aggressive polling

Medium Priority

Add unit tests for data provider functions
Clean up commented code or add TODO explanations
Document the header removal logic
Add query key constants to prevent invalidation bugs
Validate performance improvements with before/after metrics

Low Priority

Consider adding JSDoc comments for complex query options
Extract magic numbers (RECORDS_PER_PAGE) to shared constants
Add runtime type validation for API responses using Zod

Conclusion

This is a solid architectural improvement that modernizes the codebase and should improve maintainability. The removal of Jotai atoms in favor of React Query is a good move for this use case.

However, there are several bugs and performance concerns that should be addressed before merging, particularly:

The query filter logic bug
Type safety issues
Aggressive polling strategy

Overall Assessment: ✅ Approve with recommendations to address the high-priority issues.

Great work on this refactor! The codebase will be much cleaner after this lands. 🚀

pkg-pr-new · 2025-11-12T19:59:35Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 18ed019

refactor: performance improvements for inspector

18ed019

jog1t mentioned this pull request Nov 12, 2025

fix(fe): use publishable token for integrating next.js with other services #3451

Draft

vercel bot had a problem deploying to Preview – rivet-inspector November 12, 2025 19:58 Failure

vercel bot had a problem deploying to Preview – rivetkit-serverless November 12, 2025 19:58 Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: performance improvements for inspector #3452

refactor: performance improvements for inspector #3452

jog1t commented Nov 12, 2025

Uh oh!

vercel bot commented Nov 12, 2025 •

edited

Loading

Uh oh!

jog1t commented Nov 12, 2025

Uh oh!

claude bot commented Nov 12, 2025

Uh oh!

pkg-pr-new bot commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor: performance improvements for inspector #3452

Are you sure you want to change the base?

refactor: performance improvements for inspector #3452

Conversation

jog1t commented Nov 12, 2025

Uh oh!

vercel bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jog1t commented Nov 12, 2025

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 12, 2025

Code Review - PR #3452: Performance Improvements for Inspector

Overview

Summary

Detailed Findings

1. Code Quality & Architecture

✅ Good: Data Provider Pattern

⚠️ Concern: Query Key Type Safety

2. Performance Considerations

⚠️ Potential Issue: Refetch Intervals

⚠️ Potential Issue: Query Invalidation Predicate

3. Potential Bugs

🐛 Bug: Empty Query Always Returns Empty

⚠️ Concern: Actor Transformation Missing Null Checks

4. Missing Error Handling

⚠️ Concern: Generic Error Messages

⚠️ Concern: Silent Failures

5. Code Style & Best Practices

✅ Good: Consistent Patterns

⚠️ Minor: Commented Code

6. Testing Considerations

7. Performance Validation

Security Concerns

✅ Good: Auth Handling

⚠️ Note: Header Manipulation

Recommendations

High Priority

Medium Priority

Low Priority

Conclusion

Uh oh!

pkg-pr-new bot commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel bot commented Nov 12, 2025 •

edited

Loading