Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 19, 2026

Closes FRONT-916

TL;DR

Conditionally render the Settings link in the sidebar and clean up button styling.

What changed?

  • Added conditional rendering for the Settings link in the sidebar, only showing it when the user is on a namespace page
  • Removed explicit size="xs" attributes from various HeaderButton components
  • Removed redundant styling classes from several HeaderButton components
  • Added the useMatchRoute hook to check the current route for conditional rendering

How to test?

  1. Navigate to a namespace page and verify the Settings link appears in the sidebar
  2. Navigate away from a namespace page and verify the Settings link disappears
  3. Check that all buttons in the sidebar maintain consistent styling despite the removed attributes

Why make this change?

This change improves the user experience by only showing relevant navigation options based on context. The Settings link is only useful when viewing a namespace, so it should only appear in that context. Additionally, the code is cleaner by removing redundant styling attributes that are likely handled by default component styles.FRONT-916

Copy link
Contributor Author

jog1t commented Jan 19, 2026

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.

@railway-app
Copy link

railway-app bot commented Jan 19, 2026

🚅 Deployed to the rivet-pr-3979 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jan 20, 2026 at 7:06 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jan 20, 2026 at 7:05 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jan 20, 2026 at 7:01 pm

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3979 January 19, 2026 22:10 Destroyed
@jog1t jog1t self-assigned this Jan 19, 2026
@jog1t jog1t marked this pull request as ready for review January 19, 2026 22:10
@linear
Copy link

linear bot commented Jan 19, 2026

@claude
Copy link

claude bot commented Jan 19, 2026

Code Review - PR #3979

Summary

This PR conditionally renders the Settings link in the sidebar based on whether the user is on a namespace page, and cleans up redundant button styling attributes. The changes are well-targeted and address the issue described.

✅ Positive Aspects

  1. Correct Problem Resolution: The conditional rendering using useMatchRoute properly fixes the "resource not found" error by only showing the Settings link when on a namespace page.

  2. Code Cleanup: Removing redundant size="xs" and styling attributes is good, as these appear to be handled by the HeaderButton component's default styles (lines 476-487).

  3. Consistent Pattern: The use of matchRoute with fuzzy: true matches the existing pattern used elsewhere in the file (e.g., line 358).

  4. No Breaking Changes: The removal of styling props should be safe since HeaderButton has sensible defaults with variant="ghost" and appropriate padding/sizing.

🔍 Code Quality Observations

  1. Ternary Null Pattern: The code uses ? (...) : null} pattern (line 208). While functional, this could be simplified slightly using &&:

    {matchRoute({
      to: "/orgs/$organization/projects/$project/ns/$namespace",
      fuzzy: true,
    }) && (
      <HeaderLink
        to="/orgs/$organization/projects/$project/ns/$namespace/settings"
        className="font-normal"
        icon={faCog}
      >
        Settings
      </HeaderLink>
    )}

    However, the current pattern is perfectly fine and may be a team preference.

  2. Duplicate useMatchRoute Calls: The matchRoute hook is now called at the component level (line 155) but was already being used locally in other parts of the component. This is fine, but note that useMatchRoute returns a function that can be called multiple times without re-creating it, so there's no performance concern here.

  3. Inconsistent Styling Removal: Some buttons still retain asChild prop while others had additional props removed. This appears intentional based on the specific needs of each button, but it might be worth documenting why some buttons need asChild and others don't in component documentation.

🎯 Potential Improvements (Optional)

  1. Route Constant: Consider extracting the namespace route pattern to a constant since it's used in multiple places:

    const NAMESPACE_ROUTE = "/orgs/$organization/projects/$project/ns/$namespace";
  2. Testing Consideration: Ensure there are tests covering:

    • Settings link appears on namespace pages
    • Settings link does not appear on non-namespace pages (e.g., project pages)
    • Navigation works correctly from the Settings link

🐛 No Bugs Identified

The implementation looks correct. The conditional rendering properly prevents the 404 error that would occur when clicking Settings from non-namespace pages.

🔒 No Security Concerns

No security issues identified in this change.

⚡ No Performance Concerns

The addition of useMatchRoute() at the component level is negligible performance-wise and is the correct approach.

📊 Test Coverage

While I cannot see the test files in this PR, I recommend ensuring test coverage for the conditional rendering logic.

✨ Overall Assessment

This is a clean, focused PR that solves the stated problem effectively. The code follows React best practices and maintains consistency with the existing codebase patterns. The cleanup of redundant props is a nice bonus that improves code maintainability.

Recommendation: LGTM ✅ - Ready to merge pending successful CI checks.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 19, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 470992d

@jog1t jog1t force-pushed the 01-19-fix_inspector_prevent_error_flickering branch from e01279b to c997179 Compare January 20, 2026 18:53
@jog1t jog1t force-pushed the 01-19-fix_dashboard_resource_not_found_when_navigating_to_settings_on_projects_page branch from 8aa0f90 to 470992d Compare January 20, 2026 18:53
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3979 January 20, 2026 18:53 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 20, 2026

Merge activity

  • Jan 20, 7:20 PM UTC: jog1t added this pull request to the Graphite merge queue.
  • Jan 20, 7:21 PM UTC: CI is running for this pull request on a draft pull request (#3984) due to your merge queue CI optimization settings.
  • Jan 20, 7:21 PM UTC: Merged by the Graphite merge queue via draft PR: #3984.

graphite-app bot pushed a commit that referenced this pull request Jan 20, 2026
…jects page (#3979)

Closes FRONT-916

### TL;DR

Conditionally render the Settings link in the sidebar and clean up button styling.

### What changed?

- Added conditional rendering for the Settings link in the sidebar, only showing it when the user is on a namespace page
- Removed explicit `size="xs"` attributes from various HeaderButton components
- Removed redundant styling classes from several HeaderButton components
- Added the `useMatchRoute` hook to check the current route for conditional rendering

### How to test?

1. Navigate to a namespace page and verify the Settings link appears in the sidebar
2. Navigate away from a namespace page and verify the Settings link disappears
3. Check that all buttons in the sidebar maintain consistent styling despite the removed attributes

### Why make this change?

This change improves the user experience by only showing relevant navigation options based on context. The Settings link is only useful when viewing a namespace, so it should only appear in that context. Additionally, the code is cleaner by removing redundant styling attributes that are likely handled by default component styles.FRONT-916
@graphite-app graphite-app bot closed this Jan 20, 2026
@graphite-app graphite-app bot deleted the 01-19-fix_dashboard_resource_not_found_when_navigating_to_settings_on_projects_page branch January 20, 2026 19:21
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