moves config items into sidebar #1115
Conversation
|
Warning Rate limit exceeded@akshaydeo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds tab-based query-param navigation for workspace config sub-items in the sidebar; refactors the config page to render views by tab; standardizes centering/width across many config views; UI tweaks to API Keys, pricing form, logs filters, and proxy; fixes Azure Batch call name and Bedrock modelID validation. Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar as Sidebar Component
participant Router as Next.js Router
participant ConfigPage as Config Page
participant View as Config View
User->>Sidebar: Click Config sub-item (has queryParam)
Sidebar->>Sidebar: Read subItem.queryParam
Sidebar->>Router: navigate(subItem.url + "?tab=" + queryParam)
Router->>ConfigPage: Route loads with searchParams (tab)
ConfigPage->>ConfigPage: Read tab from searchParams (useSearchParams)
ConfigPage->>View: Render view matching tab (e.g., LoggingView)
View->>User: Render selected config UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/sidebar.tsx (1)
610-621: Auto-expand logic should checkqueryParamto match active state detection.The auto-expand logic at line 614 uses only
pathname.startsWith(subItem.url), but all 10 Config sub-items share the same URL (/workspace/config) with distinctqueryParamvalues. This causes the Config menu to expand whenever navigating to any config tab, which works but is semantically inconsistent with how active states are detected elsewhere.The
SidebarItemViewcomponent correctly checks both pathname and queryParam:pathname === subItem.url && currentTab === subItem.queryParamUpdate the auto-expand logic to apply the same check:
if (item.subItems?.some((subItem) => { const match = subItem.queryParam ? pathname === subItem.url && searchParams.get("tab") === subItem.queryParam : pathname.startsWith(subItem.url); return match; })) { newExpandedItems.add(item.title); }
🧹 Nitpick comments (2)
ui/app/workspace/config/views/clientSettingsView.tsx (1)
69-69: Consider standardizing Tailwind class order across config views.The layout change is functionally correct, but the class ordering varies across the config views:
- This file:
space-y-4 w-full max-w-4xl mx-auto- governanceView:
space-y-4 mx-auto w-full max-w-4xl- cachingView:
mx-auto w-full max-w-4xl space-y-4While Tailwind doesn't care about order functionally, consistent ordering improves code maintainability and makes diffs cleaner.
ui/app/workspace/config/views/pricingConfigView.tsx (1)
111-121: URL validation has a redundancy.The regex pattern already requires the URL to start with
http://orhttps://(via^(https?:\/\/)?), but the?makes the protocol optional. ThecheckIfHttpvalidator then enforces the protocol requirement separately. This creates a conflict: the regex allows URLs without a protocol, butcheckIfHttprejects them.Consider simplifying by making the protocol required in the regex pattern itself:
{...register("pricing_datasheet_url", { pattern: { - value: /^(https?:\/\/)?((localhost|(\d{1,3}\.){3}\d{1,3})(:\d+)?|([\da-z\.-]+)\.([a-z\.]{2,6}))([\/\w \.-]*)*\/?$/, + value: /^https?:\/\/((localhost|(\d{1,3}\.){3}\d{1,3})(:\d+)?|([\da-z\.-]+)\.([a-z\.]{2,6}))([\/\w \.-]*)*\/?$/, message: "Please enter a valid URL.", }, - validate: { - checkIfHttp: (value) => { - if (!value) return true; // Allow empty - return value.startsWith("http://") || value.startsWith("https://") || "URL must start with http:// or https://"; - }, - }, })}Alternatively, keep both validators if you want distinct error messages for malformed URLs vs. missing protocol.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/providers/azure/azure.go(1 hunks)ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx(1 hunks)ui/app/workspace/config/page.tsx(1 hunks)ui/app/workspace/config/views/cachingView.tsx(1 hunks)ui/app/workspace/config/views/clientSettingsView.tsx(1 hunks)ui/app/workspace/config/views/governanceView.tsx(1 hunks)ui/app/workspace/config/views/loggingView.tsx(1 hunks)ui/app/workspace/config/views/observabilityView.tsx(1 hunks)ui/app/workspace/config/views/performanceTuningView.tsx(1 hunks)ui/app/workspace/config/views/pricingConfigView.tsx(1 hunks)ui/app/workspace/config/views/proxyView.tsx(1 hunks)ui/app/workspace/config/views/securityView.tsx(1 hunks)ui/app/workspace/logs/views/filters.tsx(1 hunks)ui/components/sidebar.tsx(11 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/views/clientSettingsView.tsxui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/config/views/cachingView.tsxui/app/workspace/config/page.tsxui/app/workspace/logs/views/filters.tsxui/app/workspace/config/views/performanceTuningView.tsxui/app/workspace/config/views/pricingConfigView.tsxui/app/workspace/config/views/governanceView.tsxui/app/workspace/config/views/loggingView.tsxui/components/sidebar.tsxcore/providers/azure/azure.goui/app/workspace/config/views/observabilityView.tsxui/app/workspace/config/views/proxyView.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/azure/azure.go
🧬 Code graph analysis (5)
ui/app/workspace/config/page.tsx (1)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)
ui/app/workspace/logs/views/filters.tsx (2)
ui/components/ui/popover.tsx (1)
PopoverContent(37-37)ui/components/ui/command.tsx (3)
Command(114-114)CommandList(114-114)CommandItem(114-114)
ui/app/workspace/config/views/pricingConfigView.tsx (3)
ui/components/ui/button.tsx (1)
Button(70-70)ui/components/ui/label.tsx (1)
Label(21-21)ui/components/ui/input.tsx (1)
Input(15-69)
ui/components/sidebar.tsx (1)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)
core/providers/azure/azure.go (1)
core/schemas/batch.go (1)
BifrostBatchRetrieveRequest(150-159)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (14)
ui/app/workspace/config/views/governanceView.tsx (1)
64-64: LGTM! Layout standardization applied.The container now uses horizontal centering and a maximum width constraint, aligning with the PR's goal of standardizing configuration page layouts.
ui/app/workspace/config/views/cachingView.tsx (1)
10-10: LGTM! Consistent layout pattern applied.The centered, width-constrained layout matches the PR's standardization goals.
ui/app/workspace/config/views/securityView.tsx (1)
170-170: LGTM! Layout standardization applied.The centered, width-constrained container aligns with the PR's UI consistency goals.
ui/app/workspace/config/views/performanceTuningView.tsx (1)
102-102: LGTM! Consistent layout pattern applied.The container now uses horizontal centering and maximum width constraints, matching the PR's standardization effort.
ui/app/workspace/config/views/loggingView.tsx (1)
79-79: LGTM! Layout standardization applied.The centered, width-constrained layout aligns with the broader UI consistency improvements in this PR.
core/providers/azure/azure.go (1)
1842-1845: LGTM! Typo fixed in method call.The corrected
BatchRetrievemethod call properly invokes the function defined at line 1622. This fixes what would have been a critical compilation or runtime error.ui/app/workspace/config/views/observabilityView.tsx (1)
79-79: LGTM! Layout standardization applied.The centered, width-constrained container matches the consistent layout pattern applied across configuration views.
ui/app/workspace/logs/views/filters.tsx (1)
330-339: LGTM!The enhanced "Recalculate costs" menu item with a descriptive subtitle improves UX clarity, and the increased popover width (250px) accommodates the new two-line layout well. The implementation is clean with appropriate muted styling for the secondary text.
ui/app/workspace/config/views/pricingConfigView.tsx (1)
83-98: LGTM!The layout restructuring with
mx-auto w-full max-w-4xlaligns well with the standardized config page layouts across the PR. The header with inline action buttons (Force Sync Now, Save Changes) provides a clean, consistent UX pattern.ui/app/workspace/config/views/proxyView.tsx (1)
60-62: LGTM!The layout wrapper with
mx-auto w-full max-w-4xl space-y-4is consistent with the standardized config view layouts across this PR. The rest of the file maintains existing functionality with import/formatting normalization.ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx (1)
70-70: LGTM!The addition of
mx-auto w-full max-w-4xlto center and constrain the content width is consistent with the standardized layout pattern applied across all config views in this PR.ui/components/sidebar.tsx (3)
492-577: LGTM!The Config sub-items with
queryParam-based navigation are well-structured. The conditional inclusion of the "Proxy" item for enterprise builds via spread operator is clean, and all sub-items have consistenthasAccesschecks tied tohasSettingsAccess.
176-187: LGTM!The
isAnySubItemActivelogic correctly handles both queryParam-based and path-based sub-items. The checkpathname === subItem.url && currentTab === subItem.queryParamensures exact matching for tab-based items whilepathname.startsWith(subItem.url)preserves the existing behavior for path-based navigation.
205-211: LGTM!The
handleSubItemClickfunction cleanly handles the two navigation modes - appending?tab=for queryParam-based items and direct navigation for path-based items.
akshaydeo
force-pushed
the
12-17-moves_config_items_into_sidebar
branch
2 times, most recently
from
1c6c761 to
1d2ee7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/providers/azure/azure.go(2 hunks)core/providers/bedrock/s3.go(1 hunks)ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx(1 hunks)ui/app/workspace/config/page.tsx(2 hunks)ui/app/workspace/config/views/cachingView.tsx(1 hunks)ui/app/workspace/config/views/clientSettingsView.tsx(1 hunks)ui/app/workspace/config/views/governanceView.tsx(1 hunks)ui/app/workspace/config/views/loggingView.tsx(1 hunks)ui/app/workspace/config/views/observabilityView.tsx(1 hunks)ui/app/workspace/config/views/performanceTuningView.tsx(1 hunks)ui/app/workspace/config/views/pricingConfigView.tsx(1 hunks)ui/app/workspace/config/views/proxyView.tsx(1 hunks)ui/app/workspace/config/views/securityView.tsx(1 hunks)ui/app/workspace/logs/views/filters.tsx(2 hunks)ui/components/sidebar.tsx(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- ui/app/workspace/config/views/performanceTuningView.tsx
- ui/app/workspace/config/views/securityView.tsx
- ui/app/workspace/logs/views/filters.tsx
- core/providers/azure/azure.go
- ui/app/workspace/config/views/loggingView.tsx
- ui/app/workspace/config/views/governanceView.tsx
- ui/app/workspace/config/views/observabilityView.tsx
- ui/app/workspace/config/views/pricingConfigView.tsx
- ui/app/workspace/config/views/cachingView.tsx
- ui/app/workspace/config/views/clientSettingsView.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:
core/providers/bedrock/s3.goui/app/workspace/config/page.tsxui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsxui/app/workspace/config/views/proxyView.tsxui/components/sidebar.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/s3.go
🧬 Code graph analysis (2)
ui/app/workspace/config/views/proxyView.tsx (5)
ui/lib/types/config.ts (2)
GlobalProxyConfig(269-281)DefaultGlobalProxyConfig(284-296)core/network/http.go (1)
GlobalProxyConfig(55-68)framework/configstore/tables/config.go (1)
GlobalProxyConfig(16-29)ui/lib/types/schemas.ts (1)
globalProxyConfigSchema(625-669)ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)
ui/components/sidebar.tsx (1)
ui/lib/constants/config.ts (1)
IS_ENTERPRISE(124-124)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (8)
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx (1)
70-70: LGTM! Standardized layout applied correctly.The addition of
mx-auto w-full max-w-4xlproperly centers and constrains the configuration view, aligning with the PR's objective to standardize UI layouts across config pages. The Tailwind utilities are correct and maintain responsive behavior.ui/app/workspace/config/views/proxyView.tsx (1)
1-349: LGTM! Formatting standardization.The changes in this file are primarily formatting and style normalization (converting single quotes to double quotes, adjusting indentation and spacing, adding trailing semicolons). The layout update on Line 61 adding
max-w-4xlstandardizes the container width to match other config views in this PR.All functional behavior—form validation, data fetching, submission flow, RBAC checks, and enterprise gating—remains unchanged.
ui/components/sidebar.tsx (4)
150-150: Good addition of queryParam for tab-based navigation.The optional
queryParamfield on theSidebarIteminterface enables tab-based subitems that share the same URL path but differ by query parameter. This is a clean approach for implementing the Config section's sub-navigation.
177-187: Correct active state logic for tab-based subitems.The updated
isAnySubItemActivelogic correctly handles both:
- Query-param-based subitems: checks if
pathname === subItem.url && currentTab === subItem.queryParam- Path-based subitems: checks if
pathname.startsWith(subItem.url)This allows the sidebar to properly highlight the active Config parent item when any of its tab-based sub-items are active.
205-211: Proper navigation handling for tab-based subitems.The
handleSubItemClickfunction correctly routes tab-based subitems by appending the query parameter (?tab=${subItem.queryParam}) to the URL, while preserving the original path-based navigation for non-tab subitems.
492-577: The queryParam values in the sidebar already match the config page tab conditionals exactly. All 10 tab identifiers (client-settings, pricing-config, logging, governance, caching, observability, security, proxy, api-keys, performance-tuning) have corresponding activeTab checks in the config page, with proper enterprise-gating for the proxy tab.core/providers/bedrock/s3.go (2)
92-95: LGTM!The comment improvements enhance code readability, and the buffer initialization is correct.
88-91: Good defensive validation to prevent empty model ID.The validation at lines 88-91 correctly prevents an empty
modelIDfrom being used. However, the nil check is unreachable in the current code path—BatchCreatevalidatesrequest.Modelis not nil before dereferencingmodelIDat line 101. The empty string check provides the real value here.
akshaydeo
force-pushed
the
12-17-moves_config_items_into_sidebar
branch
from
1d2ee7d to
a60450c
Compare
akshaydeo
changed the base branch from
12-17-fixes_blur_issue_on_char_type_in_headers_table
to
graphite-base/1115
akshaydeo
force-pushed
the
12-17-moves_config_items_into_sidebar
branch
from
a60450c to
5f276bd
Compare
akshaydeo
force-pushed
the
12-17-moves_config_items_into_sidebar
branch
from
5f276bd to
0d6747b
Compare
Merge activity
|
Summary
This PR improves the UI layout of configuration pages by standardizing the width and centering content, while also removing the sidebar tabs in favor of a more organized navigation structure through the main sidebar.
Changes
BatchRetrievemethod callType 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