UN-2226 [UX/UI] Improve settings navigation and modal component styling #1569
Conversation
- Created reusable SettingsLayout component with floating sidebar navigation - Updated settings pages to use consistent sidebar with click-outside detection - Fixed settings route to redirect to platform settings (prevents empty page) - Improved modal icon sizing to 50% for better visual balance - Updated settings pages background to white 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary by CodeRabbit
WalkthroughAdds a new SettingsLayout with a sidebar and keyboard/navigation behavior, integrates it into settings pages (PlatformSettings, DefaultTriad), and changes Settings to redirect to Platform. Multiple CSS updates across the app (fonts, image sizing, scrollbar, layout spacing) and a small JSX layout change wrapping EmptyState in IslandLayout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant R as Router
participant S as Settings (redirect)
participant SL as SettingsLayout
participant NAV as Sidebar
participant P as PlatformSettings
participant T as DefaultTriad
U->>R: Visit /org/:org/settings
R->>S: Render Settings
S->>R: Navigate to /org/:org/settings/platform
R->>SL: Render SettingsLayout (activeKey=platform)
SL->>NAV: Render sidebar items
SL->>P: Render PlatformSettings
U->>NAV: Click "Default Triad"
NAV->>R: navigate(/org/:org/settings/triad)
R->>SL: Render SettingsLayout (activeKey=triad)
SL->>T: Render DefaultTriad
sequenceDiagram
autonumber
participant P as PlatformSettings
participant UI as Per-key Row
participant CM as ConfirmModal
P->>UI: Render key row (active toggle, key input, actions)
UI->>P: Click Generate/Refresh → set isLoadingIndex → async generate
UI->>CM: Click Delete → open ConfirmModal
CM->>P: Confirm Delete → set isDeletingIndex → perform delete
P-->>UI: Update UI (loading states / key list)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/input-output/data-source-card/DataSourceCard.css (1)
11-20: Consolidate duplicate.cover-imgclass definitions.The
.cover-imgclass is defined twice (lines 11-20 and 48-54) with overlapping properties (display: flex,align-items: center). This duplication can cause confusion about which styles apply and makes maintenance harder.Consolidate these into a single definition or clarify if they serve different purposes.
Apply this diff to consolidate the definitions (merge the unique properties):
.ds-card .cover-img { height: 50%; width: 50%; text-align: center; padding: 10px; background-color: transparent; display: flex; align-items: center; overflow: hidden; + position: relative; + z-index: 0; + justify-content: center; } -.cover-img { - position: relative; - z-index: 0; - align-items: center; - display: flex; - justify-content: center; -} - .ds-card.disabled { opacity: 0.7; }Also applies to: 48-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (17)
frontend/src/components/agency/display-logs/DisplayLogs.css(1 hunks)frontend/src/components/custom-tools/editable-text/EditableText.css(1 hunks)frontend/src/components/deployments/body/Body.jsx(1 hunks)frontend/src/components/input-output/data-source-card/DataSourceCard.css(2 hunks)frontend/src/components/input-output/data-source-card/DataSourceCard.jsx(1 hunks)frontend/src/components/logs-and-notifications/DisplayLogsAndNotifications.css(3 hunks)frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx(1 hunks)frontend/src/components/onboard/onBoard.css(1 hunks)frontend/src/components/pipelines-or-deployments/pipelines/Pipelines.css(0 hunks)frontend/src/components/settings/default-triad/DefaultTriad.jsx(3 hunks)frontend/src/components/settings/platform/PlatformSettings.css(1 hunks)frontend/src/components/settings/platform/PlatformSettings.jsx(2 hunks)frontend/src/components/settings/settings-layout/SettingsLayout.jsx(1 hunks)frontend/src/components/settings/settings/Settings.css(1 hunks)frontend/src/components/settings/settings/Settings.jsx(1 hunks)frontend/src/components/widgets/list-view/ListView.css(1 hunks)frontend/src/index.css(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/pipelines-or-deployments/pipelines/Pipelines.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
frontend/src/components/deployments/body/Body.jsx (1)
15-21: LGTM! Improved layout consistency.Wrapping the
EmptyStatewithIslandLayoutensures consistent styling and structure across both the empty and populated data states, aligning with the UI/UX improvements described in the PR.frontend/src/components/widgets/list-view/ListView.css (1)
62-63: Thumb sizing refinement looks good.The square 38px dimension keeps thumbnails consistent with the updated visual language; no issues spotted.
frontend/src/components/input-output/data-source-card/DataSourceCard.css (1)
43-45: LGTM!The flexbox layout with
align-items: centerproperly centers the card contents, aligning with the PR objective to fix uncentered modal icons.frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx (1)
39-39: LGTM! Typography update aligns with UI standardization.The font size reduction from 18px to 16px is consistent with the broader typography harmonization across the application.
frontend/src/components/custom-tools/editable-text/EditableText.css (1)
21-21: LGTM! Font size standardization applied.The reduction from 20px to 14px aligns with the typography standardization across other components in this PR.
frontend/src/components/agency/display-logs/DisplayLogs.css (1)
9-13: LGTM! Consistent font size improvement.The font size increase from 12px to 14px improves readability and aligns with similar changes in related log display components.
frontend/src/components/logs-and-notifications/DisplayLogsAndNotifications.css (1)
17-53: LGTM! UI consistency improvements.The changes align well with the broader UI refresh:
- White background for consistency with the new theme
- Font size increase to 14px for improved readability
- The
!importantflag on line 39 is acceptable for overriding Ant Design's default table stylesfrontend/src/components/settings/platform/PlatformSettings.css (1)
4-5: LGTM! Layout adjustments for SettingsLayout integration.The
calc(100vh - 60px)correctly accounts for the header height, and the white background aligns with the new UI theme. Note that the 60px value is coupled to the header height defined in.plt-set-head(line 15), so future header height changes would require updating both values.frontend/src/components/settings/default-triad/DefaultTriad.jsx (2)
16-16: LGTM! SettingsLayout import added.The import is correctly placed and will be used to wrap the component content.
177-225: LGTM! Clean integration with SettingsLayout.The component is properly wrapped with
SettingsLayoutusingactiveKey="triad"for navigation highlighting. This maintains all existing functionality while integrating with the new unified settings layout structure.frontend/src/index.css (1)
32-57: LGTM! Global scrollbar styling improvements.The universal selector for
scrollbar-widthandscrollbar-coloris appropriate for this use case, providing consistent scrollbar appearance across all scrollable elements. The color updates (#E8EDF2 and #D4DCE5) align with the new UI theme, and both standard and WebKit-specific properties ensure cross-browser compatibility.frontend/src/components/settings/settings/Settings.jsx (1)
1-20: No other replace-based redirects detected. Manual testing needed to ensure browser back-button behavior from/settings/platformand deep-link loads.
| width="50%" | ||
| height="50%" | ||
| preview={false} | ||
| style={{ objectFit: "contain" }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify responsive sizing works with varying icon aspect ratios.
The change from height="auto" to height="50%" shifts from aspect-ratio-preserving sizing to percentage-based sizing. While objectFit: "contain" prevents distortion, the 50% height depends on the parent container (.cover-img) having a defined height.
In the CSS, .cover-img itself uses height: 50%, which depends on .cover-container having a defined height. Since .cover-container doesn't have an explicit height in the provided CSS, verify that the layout works correctly across different viewport sizes and icon aspect ratios.
🏁 Script executed:
#!/bin/bash
# Verify the container sizing by checking if .cover-container or parent .ds-card has defined height
rg -n -C3 'cover-container|ds-card' frontend/src/components/input-output/data-source-card/DataSourceCard.cssLength of output: 618
Define a container height or revert height="auto" Percentage-based height="50%" on the image will fall back to auto since neither .cover-container nor .ds-card has a defined height. Either add an explicit height (e.g., on .cover-container) or revert the <img> back to height="auto".
🤖 Prompt for AI Agents
frontend/src/components/input-output/data-source-card/DataSourceCard.jsx around
lines 44-47: the img uses height="50%" but its parent (.cover-container or
.ds-card) has no defined height so percentage falls back to auto; fix by either
setting an explicit height on the container (e.g., add a CSS rule for
.cover-container or .ds-card with a concrete height or min-height so the 50%
computes correctly) or revert the image to height="auto" (and remove the
percentage) so it scales by intrinsic dimensions; implement whichever approach
matches the design (explicit container height if you need a fixed card size,
otherwise use height="auto" on the img).
| align-items: flex-start; | ||
| height: 100vh; | ||
| overflow-y: auto; | ||
| padding: 50px 20px 20px 20px; |
There was a problem hiding this comment.
Prevent extra scroll caused by 100vh + padding
Setting height: 100vh while adding 70px of padding makes this flex container taller than the viewport under the default content-box sizing, so users get an extra vertical scrollbar and clipped content on shorter screens. Please keep the padding within the viewport height (e.g., add box-sizing: border-box; here or switch to a calc-based height) before shipping.
🤖 Prompt for AI Agents
In frontend/src/components/onboard/onBoard.css around lines 4 to 7 the container
uses height: 100vh with 70px total vertical padding which causes an extra
scrollbar; fix by making the padding included in the height (e.g., add
box-sizing: border-box to this selector) or compute the height excluding padding
(e.g., replace 100vh with calc(100vh - 70px)); apply one of these changes so the
element never exceeds the viewport height.
| useEffect(() => { | ||
| // Redirect to platform settings if on base settings page | ||
| if (location.pathname.endsWith("/settings")) { | ||
| navigate(`/${sessionDetails?.orgName}/settings/platform`, { | ||
| replace: true, | ||
| }); | ||
| } catch (err) { | ||
| // If an error occurs while setting custom posthog event, ignore it and continue | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="settings-bg-col"> | ||
| <IslandLayout> | ||
| <div className="settings-layout"> | ||
| <div className="settings-head"> | ||
| <Typography.Text className="settings-head-typo"> | ||
| General Settings | ||
| </Typography.Text> | ||
| </div> | ||
| <div className="settings-plt"> | ||
| <Typography.Link | ||
| className="settings-plt-typo" | ||
| strong | ||
| onClick={() => navigate("platform")} | ||
| > | ||
| Platform Settings | ||
| </Typography.Link> | ||
| </div> | ||
| <div className="settings-plt"> | ||
| <Typography.Link | ||
| className="settings-plt-typo" | ||
| strong | ||
| onClick={() => navigate(`/${sessionDetails?.orgName}/users`)} | ||
| > | ||
| User Management | ||
| </Typography.Link> | ||
| </div> | ||
| <div className="settings-plt"> | ||
| <Typography.Link | ||
| className="settings-plt-typo" | ||
| strong | ||
| onClick={handleDefaultTriadClick} | ||
| > | ||
| Default Triad | ||
| </Typography.Link> | ||
| </div> | ||
| }, [location.pathname, navigate, sessionDetails?.orgName]); |
There was a problem hiding this comment.
Guard against undefined orgName in redirect.
The redirect logic may execute before sessionDetails?.orgName is populated, resulting in navigation to /undefined/settings/platform. Add a guard condition to ensure orgName exists before redirecting.
Apply this diff to add the guard:
useEffect(() => {
// Redirect to platform settings if on base settings page
- if (location.pathname.endsWith("/settings")) {
+ if (location.pathname.endsWith("/settings") && sessionDetails?.orgName) {
navigate(`/${sessionDetails?.orgName}/settings/platform`, {
replace: true,
});
}
}, [location.pathname, navigate, sessionDetails?.orgName]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| // Redirect to platform settings if on base settings page | |
| if (location.pathname.endsWith("/settings")) { | |
| navigate(`/${sessionDetails?.orgName}/settings/platform`, { | |
| replace: true, | |
| }); | |
| } catch (err) { | |
| // If an error occurs while setting custom posthog event, ignore it and continue | |
| } | |
| }; | |
| return ( | |
| <div className="settings-bg-col"> | |
| <IslandLayout> | |
| <div className="settings-layout"> | |
| <div className="settings-head"> | |
| <Typography.Text className="settings-head-typo"> | |
| General Settings | |
| </Typography.Text> | |
| </div> | |
| <div className="settings-plt"> | |
| <Typography.Link | |
| className="settings-plt-typo" | |
| strong | |
| onClick={() => navigate("platform")} | |
| > | |
| Platform Settings | |
| </Typography.Link> | |
| </div> | |
| <div className="settings-plt"> | |
| <Typography.Link | |
| className="settings-plt-typo" | |
| strong | |
| onClick={() => navigate(`/${sessionDetails?.orgName}/users`)} | |
| > | |
| User Management | |
| </Typography.Link> | |
| </div> | |
| <div className="settings-plt"> | |
| <Typography.Link | |
| className="settings-plt-typo" | |
| strong | |
| onClick={handleDefaultTriadClick} | |
| > | |
| Default Triad | |
| </Typography.Link> | |
| </div> | |
| }, [location.pathname, navigate, sessionDetails?.orgName]); | |
| useEffect(() => { | |
| // Redirect to platform settings if on base settings page | |
| if (location.pathname.endsWith("/settings") && sessionDetails?.orgName) { | |
| navigate(`/${sessionDetails?.orgName}/settings/platform`, { | |
| replace: true, | |
| }); | |
| } | |
| }, [location.pathname, navigate, sessionDetails?.orgName]); |
🤖 Prompt for AI Agents
In frontend/src/components/settings/settings/Settings.jsx around lines 10 to 17,
the redirect runs even when sessionDetails?.orgName is undefined causing
navigation to /undefined/settings/platform; add a guard so the effect only
triggers when orgName is truthy (e.g., check sessionDetails?.orgName before
calling navigate), keeping the existing dependencies and replace option
unchanged so no redirect occurs until a valid orgName exists.
- Added keyboard listener (onKeyDown) for Enter and Space keys
- Added role='button' and tabIndex={0} for proper accessibility
- Fixes: Visible, non-interactive elements with click handlers must have keyboard listener
- Fixes: Interactive elements must support tabbing, mouse, keyboard, and touch inputs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (1)
10-10: Critical: Sidebar becomes unrecoverable after the first outside click
isSidebarVisibleis set tofalseby the click-outside handler (lines 39-42) but no mechanism exists to set it back totrue. Once a user clicks anywhere in the content area, the settings navigation disappears permanently, breaking access to other settings pages.This critical issue was flagged in a previous review and remains unaddressed.
Solution 1 (recommended): Remove click-outside logic entirely and keep sidebar persistent.
-import { useEffect, useRef, useState } from "react"; +import { useRef } from "react"; import { useNavigate, useLocation } from "react-router-dom"; import PropTypes from "prop-types"; import "../settings/Settings.css"; import { useSessionStore } from "../../../store/session-store"; function SettingsLayout({ children, activeKey }) { const navigate = useNavigate(); const location = useLocation(); - const [isSidebarVisible, setIsSidebarVisible] = useState(true); const sidebarRef = useRef(null); const { sessionDetails } = useSessionStore(); const settingsMenuItems = [ { key: "platform", label: "Platform Settings", path: `/${sessionDetails?.orgName}/settings/platform`, }, { key: "users", label: "User Management", path: `/${sessionDetails?.orgName}/users`, }, { key: "triad", label: "Default Triad", path: `/${sessionDetails?.orgName}/settings/triad`, }, { key: "review", label: "Human In the Loop Settings", path: `/${sessionDetails?.orgName}/settings/review`, }, ]; - // Handle click outside to close sidebar - useEffect(() => { - const handleClickOutside = (event) => { - if (sidebarRef.current && !sidebarRef.current.contains(event.target)) { - setIsSidebarVisible(false); - } - }; - - document.addEventListener("mousedown", handleClickOutside); - return () => { - document.removeEventListener("mousedown", handleClickOutside); - }; - }, []); // Determine active key from route if not provided const getActiveKey = () => { if (activeKey) return activeKey; const path = location.pathname; if (path.includes("platform")) return "platform"; if (path.includes("users")) return "users"; if (path.includes("triad")) return "triad"; if (path.includes("review")) return "review"; return "platform"; }; const currentActiveKey = getActiveKey(); const handleKeyDown = (event, path) => { if (event.key === "Enter" || event.key === " ") { event.preventDefault(); navigate(path); } }; return ( <div className="settings-container"> - {isSidebarVisible && ( - <div className="settings-sidebar" ref={sidebarRef}> + <div className="settings-sidebar" ref={sidebarRef}> {settingsMenuItems.map((item) => ( <div key={item.key} className={`settings-menu-item ${ currentActiveKey === item.key ? "active" : "" }`} onClick={() => navigate(item.path)} onKeyDown={(e) => handleKeyDown(e, item.path)} role="button" tabIndex={0} > {item.label} </div> ))} </div> - )} <div className="settings-content">{children}</div> </div> ); } SettingsLayout.propTypes = { children: PropTypes.node.isRequired, activeKey: PropTypes.string, }; export { SettingsLayout };Solution 2 (if collapsible sidebar is required): Add an explicit toggle button.
Add a toggle button to the UI that users can click to show/hide the sidebar, and optionally retain the click-outside-to-close behavior.
Also applies to: 37-49, 73-90
🧹 Nitpick comments (1)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (1)
54-59: Route matching could produce false positives with substring includesThe active key determination uses simple substring matching (e.g.,
path.includes("platform")). This could incorrectly match unintended routes. For example, if a route contains/my-platform-configs, it would incorrectly highlight "Platform Settings".Consider using more precise route matching:
// Determine active key from route if not provided const getActiveKey = () => { if (activeKey) return activeKey; const path = location.pathname; - if (path.includes("platform")) return "platform"; - if (path.includes("users")) return "users"; - if (path.includes("triad")) return "triad"; - if (path.includes("review")) return "review"; + if (path.includes("/settings/platform")) return "platform"; + if (path.includes("/users")) return "users"; + if (path.includes("/settings/triad")) return "triad"; + if (path.includes("/settings/review")) return "review"; return "platform"; };Or use a more robust approach with exact path segments:
const getActiveKey = () => { if (activeKey) return activeKey; const path = location.pathname; + const segments = path.split('/'); - if (path.includes("platform")) return "platform"; - if (path.includes("users")) return "users"; - if (path.includes("triad")) return "triad"; - if (path.includes("review")) return "review"; + if (segments.includes("platform")) return "platform"; + if (segments.includes("users")) return "users"; + if (segments.includes("triad")) return "triad"; + if (segments.includes("review")) return "review"; return "platform"; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (3)
64-69: LGTM: Keyboard navigation properly implementedThe keyboard event handler correctly supports both Enter and Space keys for accessibility, prevents default behavior to avoid page scrolling on Space, and navigates to the appropriate path. This ensures the menu items are keyboard-accessible.
96-99: LGTM: PropTypes properly definedThe PropTypes are correctly defined with
childrenas a required node andactiveKeyas an optional string, providing runtime type checking for component usage.
14-35: Guard against undefined orgName before rendering menu links
WrapSettingsLayout(or its parent) in a loading state or early return untilsessionDetails.orgNameis defined to prevent paths like/undefined/....
| }, | ||
| { | ||
| key: "review", | ||
| label: "Human In the Loop Settings", |
There was a problem hiding this comment.
Fix typographical error: double space in label text
Line 32 contains "Human In the Loop Settings" with two consecutive spaces between "Loop" and "Settings".
{
key: "review",
- label: "Human In the Loop Settings",
+ label: "Human In the Loop Settings",
path: `/${sessionDetails?.orgName}/settings/review`,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label: "Human In the Loop Settings", | |
| { | |
| key: "review", | |
| label: "Human In the Loop Settings", | |
| path: `/${sessionDetails?.orgName}/settings/review`, | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/components/settings/settings-layout/SettingsLayout.jsx around
line 32, the label value contains a typographical error with two consecutive
spaces ("Human In the Loop Settings"); update the string to have a single space
between "Loop" and "Settings" so it reads "Human In the Loop Settings".
- Remove click-outside logic that made sidebar unrecoverable - Fix route matching to use specific paths instead of substring includes - Consolidate duplicate .cover-img CSS class definitions - Sidebar now remains persistent for consistent navigation access
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (1)
31-31: Fix typographical error: double space in label text.Line 31 contains "Human In the Loop Settings" with two consecutive spaces between "Loop" and "Settings". This issue was flagged in a previous review and remains unresolved.
Apply this diff:
{ key: "review", - label: "Human In the Loop Settings", + label: "Human In the Loop Settings", path: `/${sessionDetails?.orgName}/settings/review`, },
🧹 Nitpick comments (2)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (2)
1-1: Remove unused import and ref.The
useRefimport andsidebarRefare no longer used after the click-outside logic was removed. Cleaning up this dead code improves maintainability.Apply this diff:
-import { useRef } from "react"; +import { } from "react"; import { useNavigate, useLocation } from "react-router-dom";const navigate = useNavigate(); const location = useLocation(); - const sidebarRef = useRef(null); const { sessionDetails } = useSessionStore();- <div className="settings-sidebar" ref={sidebarRef}> + <div className="settings-sidebar">Also applies to: 10-10
37-45: Use more precise route matching.The current
.includes()checks can produce false positives. For example, a route like"/other-settings/platform-tools"would incorrectly match"/settings/platform". Consider usingstartsWithwith the full expected path prefix or exact matching.Apply this diff for more precise matching:
const getActiveKey = () => { if (activeKey) return activeKey; const path = location.pathname; - if (path.includes("/settings/platform")) return "platform"; - if (path.includes("/users")) return "users"; - if (path.includes("/settings/triad")) return "triad"; - if (path.includes("/settings/review")) return "review"; + const orgPrefix = `/${sessionDetails?.orgName}`; + if (path.startsWith(`${orgPrefix}/settings/platform`)) return "platform"; + if (path.startsWith(`${orgPrefix}/users`)) return "users"; + if (path.startsWith(`${orgPrefix}/settings/triad`)) return "triad"; + if (path.startsWith(`${orgPrefix}/settings/review`)) return "review"; return "platform"; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/input-output/data-source-card/DataSourceCard.css(2 hunks)frontend/src/components/settings/settings-layout/SettingsLayout.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (2)
49-72: Well-implemented keyboard accessibility.The keyboard navigation correctly handles Enter and Space keys with proper ARIA attributes (
role="button",tabIndex={0}), making the sidebar navigation accessible to keyboard users.
79-82: LGTM!PropTypes are correctly defined with
childrenas required andactiveKeyas an optional string, matching the component's usage patterns.
| const settingsMenuItems = [ | ||
| { | ||
| key: "platform", | ||
| label: "Platform Settings", | ||
| path: `/${sessionDetails?.orgName}/settings/platform`, | ||
| }, | ||
| { | ||
| key: "users", | ||
| label: "User Management", | ||
| path: `/${sessionDetails?.orgName}/users`, | ||
| }, | ||
| { | ||
| key: "triad", | ||
| label: "Default Triad", | ||
| path: `/${sessionDetails?.orgName}/settings/triad`, | ||
| }, | ||
| { | ||
| key: "review", | ||
| label: "Human In the Loop Settings", | ||
| path: `/${sessionDetails?.orgName}/settings/review`, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify sessionDetails availability before rendering navigation.
If sessionDetails or orgName is undefined when this component renders, the navigation paths will become invalid (e.g., "/undefined/settings/platform"), breaking navigation. Ensure that SettingsLayout only renders when valid session data is available, or add a loading/error state.
Run the following script to check how sessionDetails is initialized and whether there are guards elsewhere:
🏁 Script executed:
#!/bin/bash
# Check sessionDetails initialization and usage patterns
# Find session store definition
echo "=== Session store definition ==="
fd -e js -e jsx -e ts -e tsx . --exec rg -l "useSessionStore" | head -5
# Check for sessionDetails guards in parent components
echo -e "\n=== Checking for sessionDetails guards ==="
rg -n "sessionDetails.*orgName" --type jsx --type js -C 3
# Find where SettingsLayout is used
echo -e "\n=== SettingsLayout usage ==="
rg -n "SettingsLayout" --type jsx --type js -C 2Length of output: 355
#!/bin/bash
# Inspect session-store initialization
echo "=== session-store.js ==="
sed -n '1,200p' frontend/src/store/session-store.js
# Inspect SettingsLayout.jsx for sessionDetails guards
echo -e "\n=== SettingsLayout.jsx ==="
sed -n '1,200p' frontend/src/components/settings/settings-layout/SettingsLayout.jsx
rg -n "sessionDetails" -C3 frontend/src/components/settings/settings-layout/SettingsLayout.jsx
# Inspect App.jsx for router usage of SettingsLayout
echo -e "\n=== App.jsx Routes ==="
sed -n '1,300p' frontend/src/App.jsx | rg -n "SettingsLayout" -C3 || echo "No SettingsLayout reference in first 300 lines"
# Global search for SettingsLayout usage
echo -e "\n=== Global SettingsLayout usage ==="
rg -n "<SettingsLayout" -C3🏁 Script executed:
#!/bin/bash
echo "=== session-store.js (init) ==="
sed -n '1,200p' frontend/src/store/session-store.js
echo -e "\n=== SettingsLayout.jsx (component) ==="
sed -n '1,200p' frontend/src/components/settings/settings-layout/SettingsLayout.jsx
echo -e "\n=== sessionDetails usages in SettingsLayout.jsx ==="
rg -n "sessionDetails" -C3 frontend/src/components/settings/settings-layout/SettingsLayout.jsx
echo -e "\n=== Global sessionDetails guards ==="
rg -n "sessionDetails" -C3 frontend/src
echo -e "\n=== SettingsLayout mounting ==="
rg -n "<SettingsLayout" -C3 frontend/srcLength of output: 184248
🏁 Script executed:
#!/bin/bash
echo "=== App.jsx (start) ==="
sed -n '1,200p' frontend/src/App.jsx
echo -e "\n=== RequireAuth references in App.jsx ==="
rg -n "RequireAuth" -C5 frontend/src/App.jsx
echo -e "\n=== SettingsLayout in routes ==="
rg -n "SettingsLayout" -C5 frontend/src/App.jsxLength of output: 3247
🏁 Script executed:
#!/bin/bash
echo "=== Router.jsx (start) ==="
sed -n '1,200p' frontend/src/routes/Router.jsx
echo -e "\n=== RequireAuth in Router.jsx ==="
rg -n "RequireAuth" -C5 frontend/src/routes/Router.jsx
echo -e "\n=== Settings routes in Router.jsx ==="
rg -n "settings" -C5 frontend/src/routes/Router.jsxLength of output: 6617
Guard against missing orgName in SettingsLayout
Add a runtime check in SettingsLayout to prevent rendering when sessionDetails?.orgName is undefined (for example, return a loader or error state) before building the menu paths.
🤖 Prompt for AI Agents
In frontend/src/components/settings/settings-layout/SettingsLayout.jsx around
lines 13 to 34, the menu builds paths using sessionDetails?.orgName without
guarding for a missing orgName; add a runtime guard before constructing
settingsMenuItems (e.g., compute const orgName = sessionDetails?.orgName and if
(!orgName) return a loader or error UI) so you only build and render the menu
when orgName is defined, otherwise short-circuit rendering to a loading/error
state.
|
@athul-rs pls add screenshots. |
- Popup appears when navigating to any settings page (including re-navigation to same page) - Popup closes when clicking outside - Platform button always shows popup, even when already on Platform Settings - Uses location.key to detect navigation events (triggers even for same-path navigation) - Platform button path set to /settings/platform for direct access
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/settings/settings-layout/SettingsLayout.jsx (1)
31-33: Fix the label typoThe label still reads
"Human In the Loop Settings"with a double space. Please collapse it to a single space.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx(1 hunks)frontend/src/components/settings/settings-layout/SettingsLayout.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| // Handle click outside to close sidebar | ||
| useEffect(() => { | ||
| const handleClickOutside = (event) => { | ||
| if (sidebarRef.current && !sidebarRef.current.contains(event.target)) { | ||
| setIsSidebarVisible(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener("mousedown", handleClickOutside); | ||
| return () => { | ||
| document.removeEventListener("mousedown", handleClickOutside); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Sidebar hides permanently after any outside click
Once the mousedown handler fires, isSidebarVisible stays false, leaving the settings navigation inaccessible and blocking all in-app navigation between settings routes. Please keep the sidebar persistent (or add an explicit toggle to reopen it) before shipping. One minimal fix is to drop the outside-click handler and always render the sidebar:
-import { useEffect, useRef, useState } from "react";
+import { useEffect } from "react";
@@
- const [isSidebarVisible, setIsSidebarVisible] = useState(true);
- const sidebarRef = useRef(null);
+ const isSidebarVisible = true;
@@
- // Show sidebar when route changes (navigating to a new settings page)
- // location.key changes even when navigating to the same path
- useEffect(() => {
- setIsSidebarVisible(true);
- }, [location.pathname, location.key]);
-
- // Handle click outside to close sidebar
- useEffect(() => {
- const handleClickOutside = (event) => {
- if (sidebarRef.current && !sidebarRef.current.contains(event.target)) {
- setIsSidebarVisible(false);
- }
- };
-
- document.addEventListener("mousedown", handleClickOutside);
- return () => {
- document.removeEventListener("mousedown", handleClickOutside);
- };
- }, []);
+ useEffect(() => {}, [location.pathname, location.key]); // keep hook import until further cleanup
@@
- {isSidebarVisible && (
- <div className="settings-sidebar" ref={sidebarRef}>
+ {isSidebarVisible && (
+ <div className="settings-sidebar">
@@
- </div>
- )}
+ </div>
+ )}(This keeps the sidebar visible; feel free to follow up with a purposeful collapse control instead.)
Also applies to: 77-95
🤖 Prompt for AI Agents
frontend/src/components/settings/settings-layout/SettingsLayout.jsx around lines
43-55 (and similarly 77-95): the outside-click mousedown handler currently sets
isSidebarVisible to false and leaves the sidebar permanently hidden; remove this
effect (or disable the outside-click listener) so the sidebar is always
rendered, or replace it with a proper toggle that only toggles visibility when
an explicit control is used; update the component to stop adding/removing the
document mousedown listener and ensure the sidebar remains visible by default
(or implement a visible state that can be reopened via a button) so navigation
between settings routes is not blocked.
|
@vishnuszipstack @jagadeeswaran-zipstack Updated with Screenshots |
|
2 participants
What
Why
How
Settings Navigation
SettingsLayoutcomponent with floating sidebar menu positioned near Platform buttonModal Icon Improvements
Additional UI/UX Improvements
Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
Related Issues or PRs
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.