Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Dec 2, 2025

TL;DR

Fixed a route path and improved hook usage in the namespace connection flow.

What changed?

  • Fixed the route path in create-namespace-frame.tsx by removing the trailing slash from "/_context/" to "/_context"
  • Refactored the usePublishableToken function in the namespace connect page to avoid unnecessary hook calls when they're not needed for the current app type
  • Added appropriate biome-ignore comments to handle lint warnings for hooks used within conditional blocks

How to test?

  1. Verify that namespace creation works correctly
  2. Test the connect page in both cloud and engine contexts to ensure tokens are properly retrieved
  3. Confirm that no React hook rules are violated during runtime

Why make this change?

The trailing slash in the route path could cause routing issues. The hook refactoring improves code efficiency by only calling the relevant data provider hooks based on the current app type, rather than always calling both hooks regardless of which one is needed.

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 2, 2025 22:01 Inactive
@vercel
Copy link

vercel bot commented Dec 2, 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 Dec 2, 2025 10:04pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 2, 2025 10:04pm
rivet-site Ignored Ignored Preview Dec 2, 2025 10:04pm
rivetkit-serverless Skipped Skipped Dec 2, 2025 10:04pm

Copy link
Contributor Author

jog1t commented Dec 2, 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.

@jog1t jog1t marked this pull request as ready for review December 2, 2025 22:01
@claude
Copy link

claude bot commented Dec 2, 2025

Pull Request Review

Summary

This PR makes two changes to the frontend:

  1. Fixes a route path in the namespace creation dialog (removing trailing slash)
  2. Refactors the usePublishableToken hook to conditionally call data provider hooks based on app type

Code Quality & Best Practices

✅ Positive Observations

  1. Route Path Fix: The change from "/_context/" to "/_context" is correct. TanStack Router typically uses paths without trailing slashes for route matching, and this aligns with the route definition in _context.tsx:42.

  2. Conditional Hook Optimization: The refactored usePublishableToken function now only calls the relevant data provider hooks based on the app type, which is more efficient.

  3. Biome Ignore Comments: The addition of biome-ignore comments acknowledges the linting warnings for hooks called within conditional blocks.

⚠️ Concerns & Issues

CRITICAL - React Hooks Rules Violation

The refactored usePublishableToken function violates React's Rules of Hooks in a potentially problematic way:

function usePublishableToken() {
	return match(__APP_TYPE__)
		.with("cloud", () => {
			// Hook called conditionally inside match()
			return useSuspenseQuery(
				useCloudNamespaceDataProvider().publishableTokenQueryOptions(),
			).data;
		})
		.with("engine", () => {
			// Hook called conditionally inside match()
			return useSuspenseQuery(
				useEngineNamespaceDataProvider().engineAdminTokenQueryOptions(),
			).data;
		})
		// ...
}

Why this is problematic:

While __APP_TYPE__ is a build-time constant (which means only one branch ever executes at runtime), this pattern:

  1. Violates the fundamental contract of React hooks - Hooks must be called in the same order on every render. Even though the build constant ensures only one branch exists in the final bundle, the source code structure suggests conditional hook calls, which makes the code harder to maintain and reason about.

  2. Creates maintenance risks - Future developers might not understand that __APP_TYPE__ is a build constant and might try to use similar patterns with runtime variables, leading to bugs.

  3. Inconsistent with the codebase's existing patterns - Looking at create-namespace-frame.tsx:12-36, you have a better pattern:

const useDataProvider = () => {
	return match(__APP_TYPE__)
		.with("cloud", () => {
			return useRouteContext({ /* ... */ });
		})
		.with("engine", () => {
			return match(useRouteContext({ /* ... */ }))
				.with({ __type: "engine" }, (ctx) => ctx.dataProvider)
				.otherwise(() => { throw new Error("Invalid context"); });
		})
		.otherwise(() => { throw new Error("Invalid app type"); });
};

Here, hooks are called at the top level of the match branches, not nested inside useSuspenseQuery calls.

Recommended Solution:

Revert to a pattern similar to the original, but with early returns to avoid unnecessary data fetching:

function usePublishableToken() {
	const cloudProvider = __APP_TYPE__ === "cloud" ? useCloudNamespaceDataProvider() : null;
	const engineProvider = __APP_TYPE__ === "engine" ? useEngineNamespaceDataProvider() : null;
	
	const cloudData = __APP_TYPE__ === "cloud" 
		? useSuspenseQuery(cloudProvider!.publishableTokenQueryOptions())
		: null;
	const engineData = __APP_TYPE__ === "engine"
		? useSuspenseQuery(engineProvider!.engineAdminTokenQueryOptions())
		: null;

	return match(__APP_TYPE__)
		.with("cloud", () => cloudData!.data)
		.with("engine", () => engineData!.data)
		.otherwise(() => {
			throw new Error("Not in a valid context");
		});
}

Or even better, call the hooks at the top level unconditionally and let the build process tree-shake the unused code:

function usePublishableToken() {
	const cloudProvider = useCloudNamespaceDataProvider();
	const engineProvider = useEngineNamespaceDataProvider();
	const cloudData = useSuspenseQuery(cloudProvider.publishableTokenQueryOptions());
	const engineData = useSuspenseQuery(engineProvider.engineAdminTokenQueryOptions());

	return match(__APP_TYPE__)
		.with("cloud", () => cloudData.data)
		.with("engine", () => engineData.data)
		.otherwise(() => {
			throw new Error("Not in a valid context");
		});
}

The second approach is simpler, follows React's Rules of Hooks perfectly, and since __APP_TYPE__ is a build constant, the bundler should eliminate the dead code branch anyway.

Performance Considerations

  • Premature Optimization: The original code was likely already optimized by the bundler due to __APP_TYPE__ being a build-time constant. The refactor adds complexity without significant performance gains.
  • Suspense Queries: Both approaches use useSuspenseQuery, which handles loading states at the boundary level, so the performance difference is negligible.

Security Concerns

No security issues identified. The changes don't introduce any new security vulnerabilities.

Test Coverage

The PR description mentions manual testing steps, but there's no indication of automated tests. Consider adding:

  • Unit tests for the usePublishableToken hook
  • Integration tests for the namespace creation flow with both cloud and engine app types

Recommendation

Request Changes: The React Hooks Rules violation needs to be addressed before merging. While it might work due to build-time constant evaluation, it sets a bad precedent and creates maintenance risks. Please refactor to follow React's Rules of Hooks at the source code level.

The route path fix (create-namespace-frame.tsx) is good and can be kept as-is.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 2, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: c3fe362

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 2, 2025

Graphite Automations

"Test" took an action on this PR • (12/02/25)

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

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 2, 2025

Merge activity

  • Dec 2, 10:07 PM UTC: jog1t added this pull request to the Graphite merge queue.
  • Dec 2, 10:07 PM UTC: CI is running for this pull request on a draft pull request (#3575) due to your merge queue CI optimization settings.
  • Dec 2, 10:07 PM UTC: Merged by the Graphite merge queue via draft PR: #3575.

graphite-app bot pushed a commit that referenced this pull request Dec 2, 2025
### TL;DR

Fixed a route path and improved hook usage in the namespace connection flow.

### What changed?

- Fixed the route path in `create-namespace-frame.tsx` by removing the trailing slash from `"/_context/"` to `"/_context"`
- Refactored the `usePublishableToken` function in the namespace connect page to avoid unnecessary hook calls when they're not needed for the current app type
- Added appropriate biome-ignore comments to handle lint warnings for hooks used within conditional blocks

### How to test?

1. Verify that namespace creation works correctly
2. Test the connect page in both cloud and engine contexts to ensure tokens are properly retrieved
3. Confirm that no React hook rules are violated during runtime

### Why make this change?

The trailing slash in the route path could cause routing issues. The hook refactoring improves code efficiency by only calling the relevant data provider hooks based on the current app type, rather than always calling both hooks regardless of which one is needed.
@graphite-app graphite-app bot closed this Dec 2, 2025
@graphite-app graphite-app bot deleted the 12-02-fix_fe_get_data_based_on_the_app_type_on_connect_page branch December 2, 2025 22:07
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