Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 26, 2026

TL;DR

Implemented a context caching system to maintain stable references across navigation in the frontend.

What changed?

  • Created a new cache.ts file with factory functions to create and cache data provider contexts
  • Added context getter functions to the router context
  • Modified routes to use the cached contexts instead of creating new ones on each navigation
  • Removed unused import of CancelledError from react-query
  • Enhanced context inheritance to properly pass parent context properties to child contexts
  • Added redirect handling for inspector URLs

How to test?

  1. Navigate between different parts of the application (organizations, projects, namespaces)
  2. Verify that navigation is smooth and state is preserved when returning to previously visited routes
  3. Check that inspector URLs are properly redirected when needed
  4. Ensure all data provider functionality works as expected with the cached contexts

Why make this change?

This change improves application performance and user experience by preventing unnecessary recreation of data provider contexts during navigation. By maintaining stable references to these contexts, we avoid redundant API calls and state resets when users navigate between routes. The caching system also ensures proper inheritance of context properties, making the code more maintainable and reducing potential bugs related to missing context data.

@jog1t jog1t marked this pull request as ready for review January 26, 2026 19:45
Copy link
Contributor Author

jog1t commented Jan 26, 2026


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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 26, 2026

More templates

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4041

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 46d339e

@claude
Copy link

claude bot commented Jan 26, 2026

PR Review: feat(dashboard): improve data providers stability

Overview

This PR implements a context caching system to maintain stable references across navigation in the frontend. The approach is solid and addresses real performance issues with unnecessary context recreation during navigation.


🐛 Critical Issues

1. Debug Console.log Left in Production Code

Location: frontend/src/app/data-providers/cloud-data-provider.tsx:356

queryFn: async () => {
    console.log(client);  // ❌ Remove this
    const response = await client.namespaces.createAccessToken(
        project,
        namespace,
        { org: organization },
    );
    return response;
},

Issue: This debug statement should be removed before merging.


⚠️ Potential Issues

1. Cache Key Collision Risk in getOrCreateEngineContext

Location: frontend/src/app/data-providers/cache.ts:42-55

export function getOrCreateEngineContext(
	engineToken: (() => string) | string | (() => Promise<string>),
): EngineContext {
	const key =
		typeof engineToken === "function"
			? engineToken.toString()  // ⚠️ Function.toString() is unreliable
			: engineToken;
	const cached = engineContextCache.get(key);
	// ...
}

Problem: Using engineToken.toString() as a cache key is problematic because:

  • Different function instances with identical code will have the same .toString() but different closures
  • Two arrow functions () => ls.engineCredentials.get(getConfig().apiUrl) || "" could have identical string representations but reference different closure variables
  • This could cause unintended cache hits between different engine contexts

Recommendation: Either:

  1. Use a WeakMap for function references: const engineContextCache = new WeakMap<Function, EngineContext>()
  2. Require callers to provide an explicit cache key for function-based tokens
  3. Accept that function tokens always create new contexts (document this behavior)

2. Cache Never Invalidated

Location: frontend/src/app/data-providers/cache.ts:27-33

The caches are module-level variables that are never cleared:

let cloudContextCache: CloudContext | null = null;
const cloudNamespaceContextCache = new Map<string, CloudNamespaceContext>();
// ...

Problem:

  • Contexts persist for the entire application lifecycle
  • If credentials change (e.g., user logs out and logs in as different user), stale contexts may be used
  • Memory could accumulate if users navigate through many organizations/projects

Recommendation:

  • Add cache invalidation functions (e.g., clearCloudContext(), clearOrganizationContext(org))
  • Clear caches on logout/session changes
  • Consider using LRU cache or size limits for organization/project/namespace caches

3. getOrCreateOrganizationContext Cache Key Missing Organization from Parent

Location: frontend/src/app/data-providers/cache.ts:72-87

export function getOrCreateOrganizationContext(
	parent: CloudContext,
	organization: string,
): OrganizationContext {
	const key = organization;  // ⚠️ Should this consider the parent?
	const cached = organizationContextCache.get(key);
	if (cached) {
		return cached;
	}
	// ...
}

Question: Since there's only one CloudContext (singleton), this is likely fine. But if the architecture changes to support multiple cloud contexts, this could cause issues. Consider documenting this assumption or using ${parent.client}:${organization} if parent uniqueness is important.

4. Context Spreading May Not Preserve All Properties

Location: Multiple files, e.g., cloud-data-provider.tsx:215-218

return {
	...parent,
	client,
	organization,
	// ...
}

Concern: The spread operator (...parent) happens before the specific properties. If parent already has client or organization, they'll be overwritten by the local ones, which is correct. However, this pattern assumes all parent properties should be inherited. If future changes add non-inheritable properties to parent contexts, this could cause bugs.

Recommendation: Consider being more explicit about which properties are inherited vs. overridden, especially in documentation or TypeScript types.


🔍 Code Quality Observations

1. Good: Type Safety

The use of ReturnType<typeof createGlobalCloudContext> ensures type consistency between cache and factory functions.

2. Good: Separation of Concerns

Creating a dedicated cache.ts file keeps caching logic separate from context creation logic.

3. Good: Consistent Patterns

All getOrCreate* functions follow a similar pattern, making the code predictable and maintainable.

4. Minor: Unused Import Removal

Good cleanup removing CancelledError import in app.tsx:3.


🧪 Test Coverage Concerns

Missing Tests:

  • No tests verify cache behavior (cache hits, proper key generation)
  • No tests verify context reuse across navigation
  • No tests verify that parent properties are correctly inherited

Recommendation: Add integration tests that:

  1. Navigate between routes and verify the same context instance is reused
  2. Test cache key generation edge cases
  3. Verify proper context hierarchy (cloud → org → project → namespace)

🎯 Performance Considerations

Positive:

  • ✅ Reduces unnecessary context recreation
  • ✅ Prevents redundant API calls from re-initialization
  • ✅ Maintains stable React Query instances

Potential Concern:

  • Without cache invalidation, memory usage grows with the number of unique organizations/projects/namespaces visited
  • Consider monitoring memory usage in long-running sessions

📝 Documentation Suggestions

  1. Add JSDoc comments to cache.ts functions explaining:

    • When contexts are cached vs. recreated
    • Cache key generation strategies
    • Lifecycle of cached contexts
  2. Document in CLAUDE.md or architecture docs:

    • The caching strategy for data providers
    • When/how to clear caches (if invalidation is added)
    • Performance implications of the caching system

✅ Security Review

No security concerns identified. The caching system doesn't introduce new attack vectors, and existing authentication/authorization flows are preserved.


🎬 Summary

Overall Assessment: Good improvement that addresses real performance issues. The architecture is sound, but needs attention to:

Must Fix Before Merge:

  1. ❌ Remove console.log(client) at cloud-data-provider.tsx:356

Should Consider:

  1. ⚠️ Fix getOrCreateEngineContext cache key generation for function tokens
  2. ⚠️ Add cache invalidation mechanism (especially for logout/session changes)
  3. ⚠️ Add tests for cache behavior

Nice to Have:

  1. Add documentation/comments explaining cache lifecycle
  2. Consider memory usage monitoring for large navigation sessions

Recommendation: Approve after fixing the console.log and addressing the cache key collision issue in getOrCreateEngineContext.


Review generated by Claude Code following repository standards from CLAUDE.md

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 26, 2026

Graphite Automations

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

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

@jog1t jog1t force-pushed the 01-26-feat_dashboard_improve_data_providers_stability branch from 2154cae to 46d339e Compare January 28, 2026 19:59
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