moves config views into their own paths #1124
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMigrates workspace configuration from query-parameter tabs to path-based routes, adds individual client pages under /workspace/config/, adds a loading state to the config layout, replaces tab UI with an RBAC permission gate and redirect, updates sidebar navigation to use path prefixes, and fixes config-related link targets. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/workspace/config/governance/page.tsx (1)
1-12: Same duplication pattern noted.This page follows the same wrapper pattern as
api-keys/page.tsxandlogging/page.tsx. See the refactoring suggestion in the api-keys page review.
🧹 Nitpick comments (4)
ui/app/workspace/config/performance-tuning/page.tsx (1)
7-7: Redundant max-width constraint.The page wrapper applies
max-w-7xl, butPerformanceTuningViewinternally constrains its root div tomax-w-4xl(line 86 inperformanceTuningView.tsx). Since the inner constraint is always smaller, the outermax-w-7xlhas no effect and creates maintenance confusion.Consider removing the redundant outer max-width:
- <div className="mx-auto flex w-full max-w-7xl"> + <div className="flex w-full">Alternatively, if you want consistent spacing across all config pages, remove the max-width from the view component and manage it at the page level. Check other new config pages (api-keys, caching, security, etc.) for consistency.
ui/app/clientLayout.tsx (1)
15-15: Remove unusedSuspenseimport.The
Suspenseimport from React is not used anywhere in this file.Apply this diff to remove the unused import:
-import { Suspense, useEffect } from "react"; +import { useEffect } from "react";ui/app/workspace/config/caching/page.tsx (1)
1-12: Consider abstracting the repeated page wrapper pattern.Multiple config pages (pricing-config, client-settings, observability, caching, and others mentioned in the PR) share an identical structure:
"use client"directive- Import a View component
- Render the View inside
<div className="mx-auto flex w-full max-w-7xl">This duplication could be reduced by creating a shared
ConfigPageWrappercomponent or leveraging the layout system more effectively. However, the current approach is explicit and works well, so this is purely a maintainability consideration for future refactoring.Example approach (optional):
Create
ui/app/workspace/config/components/ConfigPageWrapper.tsx:"use client" export default function ConfigPageWrapper({ children }: { children: React.ReactNode }) { return ( <div className="mx-auto flex w-full max-w-7xl"> {children} </div> ) }Then each page becomes:
import ConfigPageWrapper from "../components/ConfigPageWrapper" import CachingView from "../views/cachingView" export default function CachingPage() { return ( <ConfigPageWrapper> <CachingView /> </ConfigPageWrapper> ) }ui/app/workspace/config/api-keys/page.tsx (1)
1-12: Consider extracting the common page wrapper pattern.This file, along with
logging/page.tsxandgovernance/page.tsx, follows an identical structure. While explicit page files are a valid Next.js pattern, you could reduce duplication by creating a shared wrapper component.Example shared wrapper at
ui/app/workspace/config/components/ConfigPageWrapper.tsx:"use client" interface ConfigPageWrapperProps { children: React.ReactNode } export default function ConfigPageWrapper({ children }: ConfigPageWrapperProps) { return ( <div className="mx-auto flex w-full max-w-7xl"> {children} </div> ) }Then each page becomes:
"use client" import APIKeysView from "@enterprise/components/api-keys/APIKeysView" +import ConfigPageWrapper from "../components/ConfigPageWrapper" export default function APIKeysPage() { - return ( - <div className="mx-auto flex w-full max-w-7xl"> - <APIKeysView /> - </div> - ) + return <ConfigPageWrapper><APIKeysView /></ConfigPageWrapper> }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx(1 hunks)ui/app/clientLayout.tsx(1 hunks)ui/app/workspace/config/api-keys/page.tsx(1 hunks)ui/app/workspace/config/caching/page.tsx(1 hunks)ui/app/workspace/config/client-settings/page.tsx(1 hunks)ui/app/workspace/config/governance/page.tsx(1 hunks)ui/app/workspace/config/layout.tsx(1 hunks)ui/app/workspace/config/logging/page.tsx(1 hunks)ui/app/workspace/config/observability/page.tsx(1 hunks)ui/app/workspace/config/page.tsx(1 hunks)ui/app/workspace/config/performance-tuning/page.tsx(1 hunks)ui/app/workspace/config/pricing-config/page.tsx(1 hunks)ui/app/workspace/config/proxy/page.tsx(1 hunks)ui/app/workspace/config/security/page.tsx(1 hunks)ui/app/workspace/config/views/securityView.tsx(1 hunks)ui/components/sidebar.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/config/security/page.tsxui/app/workspace/config/logging/page.tsxui/app/workspace/config/layout.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/governance/page.tsxui/app/workspace/config/observability/page.tsxui/app/workspace/config/proxy/page.tsxui/app/workspace/config/api-keys/page.tsxui/app/workspace/config/client-settings/page.tsxui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsxui/components/sidebar.tsxui/app/clientLayout.tsxui/app/workspace/config/performance-tuning/page.tsxui/app/workspace/config/page.tsxui/app/workspace/config/caching/page.tsx
🧬 Code graph analysis (11)
ui/app/workspace/config/security/page.tsx (1)
ui/app/workspace/config/views/securityView.tsx (1)
SecurityView(36-324)
ui/app/workspace/config/logging/page.tsx (1)
ui/app/workspace/config/views/loggingView.tsx (1)
LoggingView(28-170)
ui/app/workspace/config/layout.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/components/noPermissionView.tsx (1)
NoPermissionView(12-28)
ui/app/workspace/config/pricing-config/page.tsx (1)
ui/app/workspace/config/views/pricingConfigView.tsx (1)
PricingConfigView(17-161)
ui/app/workspace/config/governance/page.tsx (1)
ui/app/workspace/config/views/governanceView.tsx (1)
GovernanceView(26-99)
ui/app/workspace/config/proxy/page.tsx (2)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)ui/app/workspace/config/views/proxyView.tsx (1)
ProxyView(24-349)
ui/app/workspace/config/client-settings/page.tsx (1)
ui/app/workspace/config/views/clientSettingsView.tsx (1)
ClientSettingsView(26-123)
ui/components/sidebar.tsx (1)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)
ui/app/workspace/config/performance-tuning/page.tsx (1)
ui/app/workspace/config/views/performanceTuningView.tsx (1)
PerformanceTuningView(28-166)
ui/app/workspace/config/page.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/components/noPermissionView.tsx (1)
NoPermissionView(12-28)
ui/app/workspace/config/caching/page.tsx (1)
ui/app/workspace/config/views/cachingView.tsx (1)
CachingView(6-32)
🔇 Additional comments (13)
ui/app/workspace/config/performance-tuning/page.tsx (1)
1-11: LGTM! Clean page wrapper implementation.The page component correctly uses the "use client" directive and follows Next.js App Router conventions for wrapping the view component.
ui/app/workspace/config/pricing-config/page.tsx (1)
1-12: LGTM with a pattern observation.This page follows the correct client component pattern for Next.js 15. The structure is clean and consistent.
Note: This file and several others (client-settings, observability, caching) follow an identical wrapper pattern. See consolidated comment below about potential abstraction opportunity.
ui/app/workspace/config/layout.tsx (1)
3-19: LGTM! Proper loading state handling.The loading state implementation is correct:
- Permission check executes first (prevents unauthorized users from seeing the loader)
- Loading state is checked before rendering children
- Clean integration with the existing RBAC permission system
ui/app/workspace/config/proxy/page.tsx (1)
7-17: LGTM! Correct enterprise gating implementation.The redirect usage is correct for Next.js 15. The
redirect()function properly handles navigation by throwing a NEXT_REDIRECT error internally, which Next.js catches and processes.The enterprise check ensures that non-enterprise users are redirected to client settings rather than seeing a feature they cannot access.
ui/app/workspace/config/client-settings/page.tsx (1)
1-12: LGTM with a pattern observation.This page correctly implements the client component pattern for Next.js 15.
Note: See consolidated comment about the repeated wrapper pattern across multiple config pages.
ui/app/workspace/config/observability/page.tsx (1)
1-12: LGTM with a pattern observation.Clean implementation following Next.js 15 conventions.
Note: See consolidated comment about the wrapper pattern duplication.
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx (1)
55-55: LGTM! Correct path-based routing update.The link correctly updates from query parameter navigation (
?tab=security) to the new path-based routing (/security), aligning with the PR's refactoring objectives.ui/app/workspace/config/caching/page.tsx (1)
1-12: LGTM with a pattern observation.This page follows the correct implementation pattern.
Note: See consolidated comment about potential abstraction for the wrapper pattern.
ui/app/workspace/config/logging/page.tsx (1)
1-12: LGTM! Clean page wrapper following Next.js conventions.The client component wrapper is well-structured and follows the established pattern for config pages in this PR.
ui/components/sidebar.tsx (2)
238-238: LGTM! Correct active state logic for path-based routing.The subitem active detection properly handles both scenarios:
- Exact match (
pathname === subItem.url) when aqueryParamis present- Prefix match (
pathname.startsWith(subItem.url)) for standard path-based routingThis aligns correctly with the migration from query-parameter-based tabs to dedicated route paths.
481-556: LGTM! Config navigation URLs updated correctly.All configuration subitem URLs have been properly migrated to dedicated paths (
/workspace/config/{section}), aligning with the new page structure introduced in this PR. The routing structure is consistent and maintains proper access control checks.ui/app/workspace/config/views/securityView.tsx (1)
188-188: LGTM! Link correctly updated to new path-based routing.The href update from
/workspace/config?tab=api-keysto/workspace/config/api-keysaligns perfectly with the PR's migration to dedicated route paths.ui/app/workspace/config/security/page.tsx (1)
1-11: The container pattern is consistent across all config pages and intentional by design.All 11 config pages (security, observability, pricing-config, governance, logging, proxy, performance-tuning, caching, client-settings, api-keys) use the identical
<div className="mx-auto flex w-full max-w-7xl">wrapper pattern. This was introduced in the recent refactor "moves config views into their own paths" and provides a consistent, flexible layout foundation for the config suite. Theflexclass here enables the flexbox context needed for centering and future layout flexibility in conjunction withmx-autoandmax-w-7xl. No changes needed.Likely an incorrect or invalid review comment.
akshaydeo
force-pushed
the
12-17-moves_config_views_into_their_own_paths
branch
from
2efeb81 to
17b5905
Compare
Merge activity
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/workspace/config/page.tsx (1)
8-22: LGTM! Client-side redirect pattern correctly implemented.The previous issue with using
redirect()in a client component has been properly addressed usinguseRouter().replace()withinuseEffect. The permission check and redirect flow is correct for Next.js 15.Optional enhancement: Consider showing a loading indicator instead of
nullwhile the redirect is in progress to improve perceived performance:if (!hasConfigAccess) { return <NoPermissionView entity="configuration" />; } - return null; + return <FullPageLoader />;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx(1 hunks)ui/app/clientLayout.tsx(1 hunks)ui/app/workspace/config/api-keys/page.tsx(1 hunks)ui/app/workspace/config/caching/page.tsx(1 hunks)ui/app/workspace/config/client-settings/page.tsx(1 hunks)ui/app/workspace/config/governance/page.tsx(1 hunks)ui/app/workspace/config/layout.tsx(1 hunks)ui/app/workspace/config/logging/page.tsx(1 hunks)ui/app/workspace/config/observability/page.tsx(1 hunks)ui/app/workspace/config/page.tsx(1 hunks)ui/app/workspace/config/performance-tuning/page.tsx(1 hunks)ui/app/workspace/config/pricing-config/page.tsx(1 hunks)ui/app/workspace/config/proxy/page.tsx(1 hunks)ui/app/workspace/config/security/page.tsx(1 hunks)ui/app/workspace/config/views/securityView.tsx(1 hunks)ui/components/sidebar.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- ui/app/workspace/config/performance-tuning/page.tsx
- ui/components/sidebar.tsx
- ui/app/workspace/config/api-keys/page.tsx
- ui/app/workspace/config/security/page.tsx
- ui/app/workspace/config/caching/page.tsx
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/config/logging/page.tsx
- ui/app/workspace/config/views/securityView.tsx
- ui/app/workspace/config/client-settings/page.tsx
- ui/app/clientLayout.tsx
- ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/config/governance/page.tsxui/app/workspace/config/page.tsxui/app/workspace/config/layout.tsxui/app/workspace/config/observability/page.tsxui/app/workspace/config/proxy/page.tsx
🧬 Code graph analysis (4)
ui/app/workspace/config/governance/page.tsx (1)
ui/app/workspace/config/views/governanceView.tsx (1)
GovernanceView(26-99)
ui/app/workspace/config/page.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/components/noPermissionView.tsx (1)
NoPermissionView(12-28)
ui/app/workspace/config/layout.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/components/noPermissionView.tsx (1)
NoPermissionView(12-28)
ui/app/workspace/config/proxy/page.tsx (2)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)ui/app/workspace/config/views/proxyView.tsx (1)
ProxyView(24-349)
🔇 Additional comments (3)
ui/app/workspace/config/layout.tsx (1)
10-18: LGTM! Loading state pattern is well-structured.The loading state is correctly positioned after the permission check, ensuring users without access see the permission error immediately rather than a loading spinner. The hook is properly called at the top level before any conditional returns, which aligns with React 19 hook rules.
ui/app/workspace/config/governance/page.tsx (1)
1-11: LGTM! Follows the established page wrapper pattern.This page wrapper is consistent with other config section pages introduced in this PR (client-settings, security, logging, etc.). The simple composition pattern keeps responsibilities clear.
ui/app/workspace/config/observability/page.tsx (1)
1-11: LGTM! Consistent with the page wrapper pattern.This page follows the same simple composition pattern as the other config section pages in this refactoring. The approach is consistent and maintainable.
Summary
Refactored the configuration pages to use dedicated routes instead of query parameters, improving navigation and URL structure.
Changes
/workspace/config?tab=security→/workspace/config/security)Type of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No security implications.
Checklist
docs/contributing/README.mdand followed the guidelines