Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 05:47 Inactive
@vercel
Copy link

vercel bot commented Dec 1, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Error Error Dec 3, 2025 2:00am
rivetkit-serverless Error Error Dec 3, 2025 2:00am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 3, 2025 2:00am
rivet-site Ignored Ignored Preview Dec 3, 2025 2:00am

Copy link
Member Author

NathanFlurry commented Dec 1, 2025

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
Copy link

claude bot commented Dec 1, 2025

PR Review: Fix Frontend - Hide Tokens Button Outside Namespace Page

Summary

This PR conditionally hides the "Tokens" menu item in the user dropdown when not on a namespace page. The implementation correctly uses TanStack Router's useMatchRoute hook to check the current route.


Code Quality ✅

Positive aspects:

  • Clean, readable implementation following React best practices
  • Consistent with existing patterns in the file (see isMatchingProjectRoute at line 34)
  • Proper use of TanStack Router's fuzzy matching
  • Appropriate conditional rendering pattern with ternary operator

Suggestions:
The code is well-written and follows the existing patterns in the codebase. No significant improvements needed.


Potential Issues ⚠️

Route matching consideration:
The current implementation checks if the route matches:

to: "/orgs/$organization/projects/$project/ns/$namespace"

However, I notice there's also an engine namespace route pattern in the codebase (/ns/$namespace based on the route tree). If tokens should also be available on the engine namespace route, you may need to add an additional check:

const isMatchingEngineNamespaceRoute = match({
  to: "/ns/$namespace",
  fuzzy: true,
});

Then update the condition:

{(isMatchingNamespaceRoute || isMatchingEngineNamespaceRoute) ? (
  <DropdownMenuItem>...</DropdownMenuItem>
) : null}

Question: Should the Tokens menu item be available on both cloud namespace routes (/orgs/.../ns/$namespace) and engine namespace routes (/ns/$namespace)? Or is it intentionally only for cloud namespaces?


Performance ✅

No performance concerns. The useMatchRoute hook is efficient and the conditional rendering has negligible overhead.


Security ✅

No security concerns identified. The navigation uses relative path (".") and spreads existing params, which is safe.


Test Coverage ℹ️

Recommendation:
Consider adding tests to verify:

  1. Tokens menu item is visible on namespace routes
  2. Tokens menu item is hidden on non-namespace routes (e.g., project page, organization page)
  3. The modal opens correctly when the Tokens item is clicked

Example test cases:

  • User on /orgs/my-org/projects/my-project/ns/prod → should see Tokens
  • User on /orgs/my-org/projects/my-project → should NOT see Tokens
  • User on /orgs/my-org → should NOT see Tokens

Generated File Changes ℹ️

The changes to frontend/src/routeTree.gen.ts are auto-generated formatting changes (quote style from double to single quotes, indentation adjustments). This is expected and not a concern, though it does add noise to the diff.


Final Assessment

Overall: Looks Good! ✅

The implementation is clean and follows existing patterns. The main question is whether tokens should also be available on engine namespace routes. Please clarify the intended scope, and consider adding test coverage for this functionality.

Action items:

  1. ✅ Code quality - No changes needed
  2. ⚠️ Clarify: Should tokens be available on engine namespace routes (/ns/$namespace)?
  3. 📝 Consider: Add test coverage for this conditional rendering logic

@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from 91c693f to f28b5ac Compare December 1, 2025 05:57
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 1c0f3fc to 6348746 Compare December 1, 2025 05:57
@graphite-app graphite-app bot force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch 2 times, most recently from e211bb1 to 6134af8 Compare December 1, 2025 06:01
@graphite-app graphite-app bot force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 6348746 to 179a5c9 Compare December 1, 2025 06:01
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 06:01 Inactive
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 179a5c9 to 2ac6633 Compare December 1, 2025 13:59
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from 6134af8 to bded0cb Compare December 2, 2025 05:18
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 2ac6633 to 8aed309 Compare December 2, 2025 05:18
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_update_env_vars_for_dev branch from bded0cb to ee7fdbd Compare December 2, 2025 10:01
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 8aed309 to 83d000f Compare December 2, 2025 10:01
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