Conversation
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 4/5
- This PR is likely safe to merge with minimal risk, since the reported issue is a standards/compliance concern rather than a clear runtime bug.
- The main issue is in
web/src/components/admin/ClientLayout.tsx: icon imports should come fromweb/src/iconsinstead of@opal/icons, which could cause inconsistency with the frontend conventions inweb/AGENTS.md. - Given the medium severity (6/10) and high confidence (8/10), this is worth fixing soon but does not appear merge-blocking on functionality alone.
- Pay close attention to
web/src/components/admin/ClientLayout.tsx- align icon imports withweb/src/iconsto meet project frontend standards.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/src/components/admin/ClientLayout.tsx">
<violation number="1" location="web/src/components/admin/ClientLayout.tsx:12">
P2: Custom agent: **Frontend standards**
Icon usage in web/AGENTS.md requires icons to come from web/src/icons. Replace the @opal/icons import with the equivalent icon from web/src/icons (or add it there if missing) before using it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ee80b0a to
73a409e
Compare
|
Preview Deployment
|
Greptile SummaryThis PR introduces a shared Key changes:
The overall design is clean and the mobile/desktop split is handled correctly. Two minor items to clean up: a docstring example that references a non-existent Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ClientLayout
participant SidebarLayouts.Root
participant SidebarWrapper
participant AdminSidebar
Note over ClientLayout: useState(true) → sidebarFolded
alt Mobile
User->>ClientLayout: tap toggle button
ClientLayout->>ClientLayout: setSidebarFolded(false)
ClientLayout->>AdminSidebar: folded=false
AdminSidebar->>SidebarLayouts.Root: folded=false (no foldable)
SidebarLayouts.Root->>SidebarWrapper: renders fixed overlay, folded=false → translate-x-0
SidebarWrapper-->>User: sidebar slides in
User->>SidebarLayouts.Root: tap backdrop / fold button
SidebarLayouts.Root->>ClientLayout: onFoldChange(true)
ClientLayout->>ClientLayout: setSidebarFolded(true)
SidebarLayouts.Root->>SidebarWrapper: -translate-x-full
SidebarWrapper-->>User: sidebar slides out
else Desktop
Note over SidebarLayouts.Root: foldable=false → ignores folded prop
SidebarLayouts.Root->>SidebarWrapper: folded=undefined (always expanded)
SidebarWrapper-->>User: sidebar always visible
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: web/src/layouts/sidebar-layouts.tsx
Line: 13-16
Comment:
**Docstring references non-existent `useSidebarState` hook**
The example in the module docstring calls `useSidebarState()` which is not exported (or defined) anywhere in this file. A developer following this example would get a "is not a function" runtime error. The correct pattern to source `folded` / `setFolded` is either `useAppSidebarContext()` (for the app sidebar) or a plain `useState(true)` at the call site (as done in `ClientLayout`).
```suggestion
* function MySidebar() {
* const [folded, setFolded] = useState(true);
* const contentFolded = useSidebarFolded();
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/sections/sidebar/AppSidebar.tsx
Line: 195
Comment:
**`displayName` removed from memoized component**
The previous code explicitly set `MemoizedAppSidebarInner.displayName = "AppSidebar"` so the component appeared as `AppSidebar` in React DevTools and error stack traces. The new named-function form will show it as `Memo(AppSidebarInner)` instead. Consider re-adding the explicit display name to preserve the previous DevTools label:
```suggestion
const MemoizedAppSidebarInner = memo(function AppSidebarInner() {
```
Add after the closing `});`:
```ts
MemoizedAppSidebarInner.displayName = "AppSidebar";
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix(mobile): update sidebar responsivene..." | Re-trigger Greptile |
🖼️ Visual Regression Report
|
8566064 to
c71c78d
Compare
c71c78d to
e07b3ca
Compare
Description
Refactors the Sidebar into a layout components.
Notable functional change, the admin sidebar can now folded on mobile/small screens while on medium/large screens size still disallows the sidebar to be folded.
Related to https://linear.app/onyx-app/issue/ENG-3890/sidebar-breakpoints
How Has This Been Tested?
Expecting minimal behavior on large screens which is captured by playwright.
mobile behavior:

The whitespace is a little off on mobile, but this change is already quite large and resolve that is likely related to a longstanding TODO to standardize the admin pages layout, so will follow up with the change to address this.
Additional Options
Summary by cubic
Make the App and Admin sidebars slide in on mobile with a backdrop; desktop behavior is unchanged (App sidebar remains foldable). Matches Linear ENG-3890 sidebar breakpoints.
Bug Fixes
ClientLayout) to open the sidebar.Refactors
SidebarLayouts(Root,Header,Body,Footer) anduseSidebarFoldedin@/layouts/sidebar-layoutsto unify mobile overlay and desktop folding.AppSidebarandAdminSidebarto useSidebarLayouts;ClientLayoutnow owns admin fold state and passesfolded/onFoldChange.Written for commit e07b3ca. Summary will update on new commits.