feat: redesign sidebar MAR-1997#1637
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ 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). (2)
📝 WalkthroughWalkthroughRemoves cookie-backed sidebar expansion state. LeftSidebar now renders a narrow fixed column with an absolute wider panel revealed on hover. Preferences cookie schema drops menuExpd. Builder and case-detail routes stop wiring expansion state. Navigation, Nudge, and UserInfo switch to hover-driven styling. ChangesSidebar expansion/state removal and hover UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-builder/src/routes/_app/_builder.tsx (1)
34-34:⚠️ Potential issue | 🟡 MinorRemove unused
getRequest()result andisMenuExpandeddestructure (’tis sitting idle, forlorn)
const request = getRequest();is never used.isMenuExpandedis destructured from loader data but is neither returned by the loader nor referenced in the component.🧹 Suggested cleanup
-import { getRequest } from '`@tanstack/react-start/server`'; @@ - const request = getRequest(); const { user, inbox, organization, entitlements } = context.authInfo; @@ - isMenuExpanded, authProvider, sentryReplayEnabled,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app-builder/src/routes/_app/_builder.tsx` at line 34, Remove the dead code: delete the unused const request = getRequest() and remove the isMenuExpanded destructure from the component (it’s not provided by the loader nor used). If the loader function that supplies component props ever needs to return isMenuExpanded, add it there; otherwise ensure no references remain to isMenuExpanded in the component or its props. Confirm no other code depends on getRequest() in this module before removing the call.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app-builder/src/components/Layout/LeftSidebar.tsx`:
- Around line 5-14: LeftSidebar currently mounts {children} twice (in the rail
and in the off-canvas flyout), causing duplicate interactive DOM and
accessibility issues; fix by rendering the interactive children only once: keep
{children} in the primary sidebar element (the h-full w-14 rail) and remove
{children} from the off-canvas div, or if you must keep a second DOM node, make
the off-canvas copy non-interactive by adding inert and aria-hidden="true" (and
ensure any focusable elements therein get tabIndex={-1}) when it is visually
hidden; update the LeftSidebar component to reference the symbols LeftSidebar,
children, and the off-canvas div (group/nav) accordingly.
In `@packages/app-builder/src/routes/_app/_builder.tsx`:
- Around line 148-150: The code is referencing the nonexistent
leftSidebarSharp.value.expanded for Nudge collapsed; replace those references
with the actual LeftSidebar render variant or expansion boolean used in this
component (e.g., a local prop/state like leftSidebarExpanded or the variant
string such as leftSidebarVariant === 'collapsed'), and update both Nudge usages
(the one currently passing collapsed={!leftSidebarSharp.value.expanded} around
lines ~149-150 and the second around ~179-181) to use that real value or let CSS
handle collapse (remove the prop altogether). Ensure you import/derive the
correct symbol (e.g., leftSidebarExpanded or leftSidebarVariant) from the
surrounding component props/state and pass collapsed={!leftSidebarExpanded} or
collapsed={leftSidebarVariant === 'collapsed'} consistently.
---
Outside diff comments:
In `@packages/app-builder/src/routes/_app/_builder.tsx`:
- Line 34: Remove the dead code: delete the unused const request = getRequest()
and remove the isMenuExpanded destructure from the component (it’s not provided
by the loader nor used). If the loader function that supplies component props
ever needs to return isMenuExpanded, add it there; otherwise ensure no
references remain to isMenuExpanded in the component or its props. Confirm no
other code depends on getRequest() in this module before removing the call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0ee1301-7588-47e8-a517-9b0e5ddc4d5b
📒 Files selected for processing (4)
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/utils/preferences-cookies/config.ts
💤 Files with no reviewable changes (1)
- packages/app-builder/src/utils/preferences-cookies/config.ts
📜 Review details
⏰ 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: e2e
🧰 Additional context used
📓 Path-based instructions (3)
packages/app-builder/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/app-builder/src/**/*.{ts,tsx}: Use internal imports from@app-buildernamespace for models, queries, components, and utilities
Use ui-design-system package for UI components (Button, Modal, Select) and utility functions (cn)
Use TanStack Query hooks with naming convention useGetXyzQuery for data fetching operations
Use ts-pattern for pattern matching with the match function instead of conditional logic
Use TanStack Form for form handling instead of manual form state management
Files:
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Tailwind CSS 4 with the tailwind-preset package for consistent styling across packages
Files:
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
packages/app-builder/src/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
packages/app-builder/src/routes/**/*.tsx: Use TanStack Router file-based routing with underscore prefix for layout routes and dollar sign prefix for dynamic segments
Define routes using createFileRoute() with staticData, loader, and component options
Use staticData.BreadCrumbs (array of render functions) for breadcrumb navigation in routes
Define loaders inline using createServerFn().middleware([authMiddleware]).handler() and pass to route's loader option
Files:
packages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
🧠 Learnings (3)
📚 Learning: 2026-05-11T13:00:53.337Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1503
File: packages/app-builder/src/components/ContinuousScreening/context/ListAndTopicDatasetConfigurationBridge.tsx:13-20
Timestamp: 2026-05-11T13:00:53.337Z
Learning: In checkmarble/marble-frontend, calls to `createSharp` from the `sharpstate` library should be treated as if they were a React hook. In React `.tsx` components, call `createSharp` unconditionally at the top level of the component function body (not inside conditionals or nested functions). Do not place `createSharp` inside `useMemo`, `useCallback`, `useEffect`, or any other hook, and do not suggest wrapping it in `useMemo`—that is incorrect and should be flagged during review.
Applied to files:
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
📚 Learning: 2026-06-04T14:37:05.664Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1603
File: packages/app-builder/src/components/Settings/ScreeningProviders/ScreeningProvidersSettingsPage.tsx:103-105
Timestamp: 2026-06-04T14:37:05.664Z
Learning: When using the `Callout` component from `app-builder/components/Callout`, you do not need to wrap the component in a conditional just to avoid an empty box. `Callout` is established to render `null` (nothing) when it receives no children, so passing `children={null}` or `children={undefined}` is safe and will result in no visible output.
Applied to files:
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
📚 Learning: 2026-05-12T19:51:39.619Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-frontend PR: 1522
File: packages/app-builder/src/components/Cases/CaseAlerts.tsx:449-449
Timestamp: 2026-05-12T19:51:39.619Z
Learning: In React (.tsx) files, when rendering translated strings that include dynamic count values, always use i18n interpolation rather than appending the count as a separate raw React text node. Prefer `t('translation.key', { count })` (or the project’s equivalent) and include `{{count}}` (or the interpolation placeholder expected by the i18n setup) inside the translation string so each locale controls placement/order. Avoid patterns like `t('key') + ' (' + count + ')'` or rendering `t('key')` followed by `(${count})` as separate nodes, since this can break RTL layout (e.g., Arabic).
Applied to files:
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/routes/_app/_builder.tsx
🪛 GitHub Actions: Pull request CI / 0_check _ main.txt
packages/app-builder/src/routes/_app/_builder.tsx
[error] 149-149: Biome lint/correctness/noUndeclaredVariables: 'leftSidebarSharp' is undeclared.
[error] 180-180: Biome lint/correctness/noUndeclaredVariables: 'leftSidebarSharp' is undeclared.
[error] 34-34: Biome lint/correctness/noUnusedVariables (FIXABLE): Variable 'request' is unused. Suggested fix: prepend with underscore (e.g., '_request').
[error] 90-90: Biome lint/correctness/noUnusedVariables: Variable 'isMenuExpanded' is unused.
🪛 GitHub Actions: Pull request CI / check _ main
packages/app-builder/src/routes/_app/_builder.tsx
[error] 149-149: Biome lint error (lint/correctness/noUndeclaredVariables): 'leftSidebarSharp' variable is undeclared.
[error] 180-180: Biome lint error (lint/correctness/noUndeclaredVariables): 'leftSidebarSharp' variable is undeclared.
[error] 34-34: Biome lint error (lint/correctness/noUnusedVariables, FIXABLE): Variable 'request' is unused. Unsafe fix: rename to '_request'.
[error] 90-90: Biome lint error (lint/correctness/noUnusedVariables): Variable 'isMenuExpanded' is unused.
🪛 GitHub Check: check / main
packages/app-builder/src/routes/_app/_builder.tsx
[failure] 180-180: lint/correctness/noUndeclaredVariables
The leftSidebarSharp variable is undeclared.
[failure] 149-149: lint/correctness/noUndeclaredVariables
The leftSidebarSharp variable is undeclared.
🔇 Additional comments (3)
packages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsx (3)
1-28: LGTM!
29-86: LGTM!
115-219: A clean removal, well and truly done!The sidebar expansion state hath been purged completely, and
drawerContentModenow stands alone, managed solely through local state as befits this refactor.LGTM!
ChibiBlasphem
force-pushed
the
feat/redesign-sidebar-MAR-1997
branch
from
5c12d31 to
2566a1d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app-builder/src/components/Navigation.tsx (1)
43-43: ⚡ Quick winAdd motion-reduce utilities to
SidebarLinkandSidebarButtoninNavigation.tsx.Both
SidebarLink(line 43) andSidebarButton(line 65) use the hover-driven opacity transition but lack themotion-reduce:delay-0 motion-reduce:duration-0utilities thatLeftSidebarandSidebarNudgeinclude. For consistency and accessibility, consider adding these utilities to both label spans.Methinks the sidebar's dance is fair,
Yet motion-reduced souls deserve their care.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app-builder/src/components/Navigation.tsx` at line 43, The span used for the label in both SidebarLink and SidebarButton (the element with class "line-clamp-1 text-start opacity-0 transition-opacity group-hover/sidebar:opacity-100 delay-300 group-hover/sidebar:delay-0") should include accessibility utilities for reduced motion; update these spans in SidebarLink and SidebarButton to also add "motion-reduce:delay-0 motion-reduce:duration-0" so the opacity transition and delay are disabled when users request reduced motion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/app-builder/src/components/Navigation.tsx`:
- Line 43: The span used for the label in both SidebarLink and SidebarButton
(the element with class "line-clamp-1 text-start opacity-0 transition-opacity
group-hover/sidebar:opacity-100 delay-300 group-hover/sidebar:delay-0") should
include accessibility utilities for reduced motion; update these spans in
SidebarLink and SidebarButton to also add "motion-reduce:delay-0
motion-reduce:duration-0" so the opacity transition and delay are disabled when
users request reduced motion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a1ced73-7aa5-4438-8aba-14de9c5ee7bf
📒 Files selected for processing (7)
packages/app-builder/src/components/Layout/LeftSidebar.tsxpackages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/components/Nudge.tsxpackages/app-builder/src/components/UserInfo.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsxpackages/app-builder/src/utils/preferences-cookies/config.ts
💤 Files with no reviewable changes (1)
- packages/app-builder/src/utils/preferences-cookies/config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app-builder/src/components/UserInfo.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app-builder/src/components/Layout/LeftSidebar.tsx
- packages/app-builder/src/routes/_app/_builder/cases/_detail/s.$caseId/old.tsx
📜 Review details
⏰ 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). (2)
- GitHub Check: check / main
- GitHub Check: e2e
🧰 Additional context used
📓 Path-based instructions (3)
packages/app-builder/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/app-builder/src/**/*.{ts,tsx}: Use internal imports from@app-buildernamespace for models, queries, components, and utilities
Use ui-design-system package for UI components (Button, Modal, Select) and utility functions (cn)
Use TanStack Query hooks with naming convention useGetXyzQuery for data fetching operations
Use ts-pattern for pattern matching with the match function instead of conditional logic
Use TanStack Form for form handling instead of manual form state management
Files:
packages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/components/Nudge.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Tailwind CSS 4 with the tailwind-preset package for consistent styling across packages
Files:
packages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/components/Nudge.tsx
packages/app-builder/src/routes/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
packages/app-builder/src/routes/**/*.tsx: Use TanStack Router file-based routing with underscore prefix for layout routes and dollar sign prefix for dynamic segments
Define routes using createFileRoute() with staticData, loader, and component options
Use staticData.BreadCrumbs (array of render functions) for breadcrumb navigation in routes
Define loaders inline using createServerFn().middleware([authMiddleware]).handler() and pass to route's loader option
Files:
packages/app-builder/src/routes/_app/_builder.tsx
🧠 Learnings (3)
📚 Learning: 2026-05-11T13:00:53.337Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1503
File: packages/app-builder/src/components/ContinuousScreening/context/ListAndTopicDatasetConfigurationBridge.tsx:13-20
Timestamp: 2026-05-11T13:00:53.337Z
Learning: In checkmarble/marble-frontend, calls to `createSharp` from the `sharpstate` library should be treated as if they were a React hook. In React `.tsx` components, call `createSharp` unconditionally at the top level of the component function body (not inside conditionals or nested functions). Do not place `createSharp` inside `useMemo`, `useCallback`, `useEffect`, or any other hook, and do not suggest wrapping it in `useMemo`—that is incorrect and should be flagged during review.
Applied to files:
packages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/components/Nudge.tsx
📚 Learning: 2026-06-04T14:37:05.664Z
Learnt from: william-schlegel
Repo: checkmarble/marble-frontend PR: 1603
File: packages/app-builder/src/components/Settings/ScreeningProviders/ScreeningProvidersSettingsPage.tsx:103-105
Timestamp: 2026-06-04T14:37:05.664Z
Learning: When using the `Callout` component from `app-builder/components/Callout`, you do not need to wrap the component in a conditional just to avoid an empty box. `Callout` is established to render `null` (nothing) when it receives no children, so passing `children={null}` or `children={undefined}` is safe and will result in no visible output.
Applied to files:
packages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/components/Nudge.tsx
📚 Learning: 2026-05-12T19:51:39.619Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-frontend PR: 1522
File: packages/app-builder/src/components/Cases/CaseAlerts.tsx:449-449
Timestamp: 2026-05-12T19:51:39.619Z
Learning: In React (.tsx) files, when rendering translated strings that include dynamic count values, always use i18n interpolation rather than appending the count as a separate raw React text node. Prefer `t('translation.key', { count })` (or the project’s equivalent) and include `{{count}}` (or the interpolation placeholder expected by the i18n setup) inside the translation string so each locale controls placement/order. Avoid patterns like `t('key') + ' (' + count + ')'` or rendering `t('key')` followed by `(${count})` as separate nodes, since this can break RTL layout (e.g., Arabic).
Applied to files:
packages/app-builder/src/components/Navigation.tsxpackages/app-builder/src/routes/_app/_builder.tsxpackages/app-builder/src/components/Nudge.tsx
🔇 Additional comments (9)
packages/app-builder/src/routes/_app/_builder.tsx (4)
82-92: Well-designed hover-driven nudge wrapper.The
SidebarNudgehelper elegantly encapsulates the hover-driven positioning and sizing transitions. The delay pattern (delay-300 group-hover/sidebar:delay-0) correctly mirrors theLeftSidebarcomponent's timing, and themotion-reduceutilities ensure accessibility compliance.LGTM!
146-154: Consistent hover-driven styling for disabled nav items.The inline label styling for disabled navigation items (
opacity-0 transition-opacity group-hover/sidebar:opacity-100 delay-300 group-hover/sidebar:delay-0) correctly matches the pattern inSidebarLink. This maintains visual consistency between enabled and disabled states.LGTM!
Also applies to: 171-180
115-244: LGTM!
32-68: LGTM!Also applies to: 94-96
packages/app-builder/src/components/Navigation.tsx (1)
20-31: LGTM!Also applies to: 78-99
packages/app-builder/src/components/Nudge.tsx (4)
9-15: Clean API simplification.Removing the
collapsedprop and shifting positioning responsibility to callers viaclassNameandiconClassis a good design choice. TheSidebarNudgewrapper in_builder.tsxdemonstrates how callers can now compose the hover-driven behavior externally. The context snippet fromapi-keys.tsxconfirms existing callers don't rely on the removed prop.LGTM!
Also applies to: 27-27
17-25: LGTM!
31-42: LGTM!
44-97: LGTM!
ChibiBlasphem
force-pushed
the
feat/redesign-sidebar-MAR-1997
branch
from
2566a1d to
954a3c0
Compare
2 participants
Redesign sidebar to expand on hover. Expended version doesn't impact the layout anymore.
Specs:
Removed the the sidebar context which was handling sidebar expansion persistance and control.
Summary by CodeRabbit
Refactor
UI Behavior