Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReworks Sidebar styling and collapsed behavior (uses CSS variables, preserves layout via visibility/off-screen techniques, adds reduced-motion handling), adjusts Sidebar item/footer semantics and tooltip behavior, adds demo preview layout/classes and icon exports, updates demos/docs/tests, and replaces a placeholder icon with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…and add new icons to Sidebar
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/www/src/components/demo/demo-preview.tsx (2)
88-103: Remove commented-out code.This block appears to be leftover from a refactor. Dead code should be removed to keep the codebase clean. If needed for reference, it can be retrieved from version control.
🗑️ Remove these lines
-{ - /* // {Array.isArray(codePreview) ? ( */ -} -// <Tabs -// items={codePreview.map(tab => tab.label)} -// className={styles.codeTabGroup} -// > -// {codePreview.map(tab => ( -// <Tab className={styles.codeTab} value={tab.label} key={tab.label}> -// <Editor code={tab.code} /> -// </Tab> -// ))} -// </Tabs> -// ) : ( -// <Editor code={previewCode} /> -// )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/demo-preview.tsx` around lines 88 - 103, Remove the leftover commented-out JSX block that checks Array.isArray(codePreview) and renders <Tabs>, <Tab>, and <Editor> variants; specifically delete the commented lines referencing codePreview.map, Tabs, Tab, Editor, previewCode and styles.codeTabGroup/codeTab so only the current active render remains—this cleans up dead/refactor leftovers while keeping existing functional JSX intact.
43-57: Type assertion bypasses TypeScript safety.The cast
previewClassName as keyof typeof stylesassumes the string is always a valid key. Whilecx()handlesundefinedgracefully, this masks potential typos inpreviewClassNamevalues at compile time.♻️ Consider a type-safe approach
Define the allowed values explicitly in the types file:
// In types.ts export type PreviewClassName = 'previewTop' | 'previewSidebar'; export type DemoPreviewProps = { // ... previewClassName?: PreviewClassName; };Then in demo-preview.tsx, the lookup becomes type-safe without the assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/demo-preview.tsx` around lines 43 - 57, The code uses a unsafe type assertion previewClassName as keyof typeof styles inside the cx(...) call which bypasses TypeScript checks; update the component props to accept a narrower union type (e.g., export type PreviewClassName = 'previewTop' | 'previewSidebar' and include previewClassName?: PreviewClassName on DemoPreviewProps in your types file) and remove the cast so the cx(styles.preview, previewClassName && styles[previewClassName]) lookup is type-safe; adjust any imports/usages of DemoPreviewProps/previewClassName and keep the conditional mapping for Preview's className (previewTop/previewSidebar) as-is but rely on the new typed previewClassName instead of asserting.apps/www/src/components/demo/styles.module.css (2)
73-81: Redundantpadding-bottomdeclaration.
padding: 0 !importanton line 74 already sets all padding sides to 0, makingpadding-bottom: 0 !importanton line 75 redundant.♻️ Suggested fix
.previewContentSidebar { padding: 0 !important; - padding-bottom: 0 !important; margin-bottom: 0 !important; height: 100%; width: 100%; display: flex; flex-direction: column; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 73 - 81, The CSS for the .previewContentSidebar class has a redundant padding declaration: remove the explicit "padding-bottom: 0 !important" since "padding: 0 !important" already sets all sides to zero; update the .previewContentSidebar block (the class name previewContentSidebar) to drop the redundant padding-bottom line so styling is simpler and avoids duplicated !important rules.
55-93: Consider extracting the repeated480pxheight to a CSS variable.The PR description mentions replacing hardcoded values with CSS variables. The
480pxvalue appears three times (lines 60, 61, 92). Extracting it would improve maintainability and align with the stated objective.♻️ Suggested approach
Define a local variable at the top of the relevant selectors or use a design token:
+/* At file level or in :root */ +/* --preview-sidebar-height: 480px; */ + .previewSidebar { display: flex; flex-direction: row; align-items: stretch; justify-content: flex-start; - min-height: 480px; - height: 480px; + min-height: var(--preview-sidebar-height, 480px); + height: var(--preview-sidebar-height, 480px); width: 100%; padding: 0; } /* ... */ .previewSidebar aside { - height: 480px; + height: var(--preview-sidebar-height, 480px); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 55 - 93, Extract the repeated 480px value into a CSS variable and use that variable wherever the fixed height appears: define something like --preview-height: 480px at the top scope used by these selectors (e.g., :root or the component wrapper) and replace the hardcoded 480px in .previewSidebar (min-height/height), .previewContentSidebar child rules if present, and .previewSidebar aside to reference var(--preview-height); update any comments if needed to reflect the variable usage so all three occurrences (.previewSidebar, .previewContentSidebar > * and .previewSidebar aside) use the single token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/www/src/components/demo/demo-preview.tsx`:
- Around line 88-103: Remove the leftover commented-out JSX block that checks
Array.isArray(codePreview) and renders <Tabs>, <Tab>, and <Editor> variants;
specifically delete the commented lines referencing codePreview.map, Tabs, Tab,
Editor, previewCode and styles.codeTabGroup/codeTab so only the current active
render remains—this cleans up dead/refactor leftovers while keeping existing
functional JSX intact.
- Around line 43-57: The code uses a unsafe type assertion previewClassName as
keyof typeof styles inside the cx(...) call which bypasses TypeScript checks;
update the component props to accept a narrower union type (e.g., export type
PreviewClassName = 'previewTop' | 'previewSidebar' and include
previewClassName?: PreviewClassName on DemoPreviewProps in your types file) and
remove the cast so the cx(styles.preview, previewClassName &&
styles[previewClassName]) lookup is type-safe; adjust any imports/usages of
DemoPreviewProps/previewClassName and keep the conditional mapping for Preview's
className (previewTop/previewSidebar) as-is but rely on the new typed
previewClassName instead of asserting.
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 73-81: The CSS for the .previewContentSidebar class has a
redundant padding declaration: remove the explicit "padding-bottom: 0
!important" since "padding: 0 !important" already sets all sides to zero; update
the .previewContentSidebar block (the class name previewContentSidebar) to drop
the redundant padding-bottom line so styling is simpler and avoids duplicated
!important rules.
- Around line 55-93: Extract the repeated 480px value into a CSS variable and
use that variable wherever the fixed height appears: define something like
--preview-height: 480px at the top scope used by these selectors (e.g., :root or
the component wrapper) and replace the hardcoded 480px in .previewSidebar
(min-height/height), .previewContentSidebar child rules if present, and
.previewSidebar aside to reference var(--preview-height); update any comments if
needed to reflect the variable usage so all three occurrences (.previewSidebar,
.previewContentSidebar > * and .previewSidebar aside) use the single token.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/www/src/components/demo/demo-preview.tsxapps/www/src/components/demo/demo.tsxapps/www/src/components/demo/styles.module.cssapps/www/src/components/demo/types.tsapps/www/src/content/docs/components/sidebar/demo.ts
…tate styling for sr-only
…st for collapsed state accessibility
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)
packages/raystack/components/sidebar/sidebar-item.tsx (1)
62-67:⚠️ Potential issue | 🟠 MajorRemove
role="listitem"from the anchor element.Per ARIA-in-HTML specifications,
role="listitem"is not a permitted role on interactive<a href>elements and violates accessibility standards. User agents may ignore the invalid role or produce inconsistent behavior across assistive technologies.Recommended fix
{ ref, className: cx(styles['nav-item'], classNames?.root), 'data-active': active, 'data-disabled': disabled, - role: 'listitem', 'aria-current': active ? 'page' : undefined, 'aria-disabled': disabled, ...props },If list semantics are required, apply
role="listitem"to a non-interactive wrapper element (not the<a>itself), or use native<li>elements in the parent list structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-item.tsx` around lines 62 - 67, The anchor element in SidebarItem (props passed including ref, className: cx(styles['nav-item'], classNames?.root), 'data-active', 'data-disabled') incorrectly sets role="listitem"; remove the role attribute from the interactive <a> element in the SidebarItem component (delete the role: 'listitem' entry) and, if list semantics are needed, move that role to a non-interactive wrapper or switch the parent to use native <li> elements instead—update SidebarItem rendering accordingly (look for the ref/className/aria-current usage to locate the element).
🧹 Nitpick comments (3)
apps/www/src/components/demo/styles.module.css (1)
55-63: Consolidate repeated preview height into a CSS variable.
480pxis duplicated in multiple selectors. A local CSS custom property will make future adjustments safer.♻️ Proposed refactor
.previewSidebar { + --preview-sidebar-height: 480px; display: flex; flex-direction: row; align-items: stretch; justify-content: flex-start; - min-height: 480px; - height: 480px; + min-height: var(--preview-sidebar-height); + height: var(--preview-sidebar-height); width: 100%; padding: 0; } @@ .previewSidebar aside { - height: 480px; + height: var(--preview-sidebar-height); }Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 55 - 63, Consolidate the repeated 480px value by introducing a local CSS custom property (e.g., --preview-height) and use it wherever 480px is currently hardcoded: define the variable on a suitable container (for example :root or the component root selector) and replace the literal 480px in .previewSidebar and the other selectors referenced around lines 90-93 with var(--preview-height); ensure both min-height and height properties use the variable so future adjustments require changing only the single custom property.packages/raystack/components/sidebar/__tests__/sidebar.test.tsx (1)
249-253: Strengthen collapsed-state assertion with role-based query.Current check only verifies text presence. Add a role query so accessibility regressions on item semantics are caught.
✅ Suggested test addition
it('keeps label in DOM when collapsed (sr-only for screen readers)', () => { render(<BasicSidebar open={false} />); expect(screen.getByText(DASHBOARD_ITEM_TEXT)).toBeInTheDocument(); + expect( + screen.getByRole('link', { name: DASHBOARD_ITEM_TEXT }) + ).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/__tests__/sidebar.test.tsx` around lines 249 - 253, The test "keeps label in DOM when collapsed (sr-only for screen readers)" only asserts text presence; update it to assert via role+name to catch accessibility regressions: render BasicSidebar(open={false}) and replace/add the text-only expectation with a role query that matches the semantic element (for example use screen.getByRole('link' /* or 'listitem' if appropriate */, { name: DASHBOARD_ITEM_TEXT }) and assert toBeInTheDocument()); reference BasicSidebar and DASHBOARD_ITEM_TEXT to locate the test and ensure the role chosen matches how the sidebar items are rendered.apps/www/src/content/docs/components/sidebar/demo.ts (1)
17-286: Consider reducing repeated sidebar demo markup.Most demo exports repeat the same sidebar skeleton. A shared snippet builder (parametrized by
open,position,collapsible,hideCollapsedItemTooltip) would reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/sidebar/demo.ts` around lines 17 - 286, Multiple demo exports (preview, positionDemo, stateDemo, tooltipDemo, collapsibleDemo, hideTooltipDemo) duplicate the same Sidebar markup; create a shared builder function (e.g., buildSidebarSnippet or sidebarSnippet) that accepts parameters open, defaultOpen, position, collapsible, hideCollapsedItemTooltip and returns the composed sidebar string, then replace each export's inline sidebarLayout/sidebarLayoutRight call with a call to that builder using the appropriate options (ensure builder includes Header/Main/Footer skeleton and supports right-position wrapper via a position parameter or separate wrapper function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/raystack/components/sidebar/sidebar.module.css`:
- Around line 127-135: Replace the deprecated clip declaration in the CSS rule
that currently contains "clip: rect(0, 0, 0, 0);" with the modern
screen-reader-only pattern by using "clip-path: inset(50%);" (keep the other
properties like position, width, height, padding, margin, overflow, white-space,
and border intact); locate the rule by searching for the declaration "clip:
rect(0, 0, 0, 0);" and swap it to "clip-path: inset(50%);" to satisfy
stylelint's property-no-deprecated rule.
---
Outside diff comments:
In `@packages/raystack/components/sidebar/sidebar-item.tsx`:
- Around line 62-67: The anchor element in SidebarItem (props passed including
ref, className: cx(styles['nav-item'], classNames?.root), 'data-active',
'data-disabled') incorrectly sets role="listitem"; remove the role attribute
from the interactive <a> element in the SidebarItem component (delete the role:
'listitem' entry) and, if list semantics are needed, move that role to a
non-interactive wrapper or switch the parent to use native <li> elements
instead—update SidebarItem rendering accordingly (look for the
ref/className/aria-current usage to locate the element).
---
Nitpick comments:
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 55-63: Consolidate the repeated 480px value by introducing a local
CSS custom property (e.g., --preview-height) and use it wherever 480px is
currently hardcoded: define the variable on a suitable container (for example
:root or the component root selector) and replace the literal 480px in
.previewSidebar and the other selectors referenced around lines 90-93 with
var(--preview-height); ensure both min-height and height properties use the
variable so future adjustments require changing only the single custom property.
In `@apps/www/src/content/docs/components/sidebar/demo.ts`:
- Around line 17-286: Multiple demo exports (preview, positionDemo, stateDemo,
tooltipDemo, collapsibleDemo, hideTooltipDemo) duplicate the same Sidebar
markup; create a shared builder function (e.g., buildSidebarSnippet or
sidebarSnippet) that accepts parameters open, defaultOpen, position,
collapsible, hideCollapsedItemTooltip and returns the composed sidebar string,
then replace each export's inline sidebarLayout/sidebarLayoutRight call with a
call to that builder using the appropriate options (ensure builder includes
Header/Main/Footer skeleton and supports right-position wrapper via a position
parameter or separate wrapper function).
In `@packages/raystack/components/sidebar/__tests__/sidebar.test.tsx`:
- Around line 249-253: The test "keeps label in DOM when collapsed (sr-only for
screen readers)" only asserts text presence; update it to assert via role+name
to catch accessibility regressions: render BasicSidebar(open={false}) and
replace/add the text-only expectation with a role query that matches the
semantic element (for example use screen.getByRole('link' /* or 'listitem' if
appropriate */, { name: DASHBOARD_ITEM_TEXT }) and assert toBeInTheDocument());
reference BasicSidebar and DASHBOARD_ITEM_TEXT to locate the test and ensure the
role chosen matches how the sidebar items are rendered.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/www/src/components/demo/styles.module.cssapps/www/src/content/docs/components/sidebar/demo.tsapps/www/src/content/docs/components/sidebar/index.mdxpackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar-item.tsxpackages/raystack/components/sidebar/sidebar-misc.tsxpackages/raystack/components/sidebar/sidebar.module.css
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); | ||
| white-space: nowrap; | ||
| border: 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
cat -n packages/raystack/components/sidebar/sidebar.module.css | sed -n '120,140p'Repository: raystack/apsara
Length of output: 556
🏁 Script executed:
# Verify the deprecated clip property using the provided script
rg -nP '^\s*clip\s*:' packages/raystack/components/sidebar/sidebar.module.cssRepository: raystack/apsara
Length of output: 88
🏁 Script executed:
# Check for any stylelint configuration files
find . -maxdepth 2 -name '.stylelintrc*' -o -name 'stylelint.config.*' 2>/dev/null | head -20Repository: raystack/apsara
Length of output: 78
🏁 Script executed:
# Search for any existing clip-path usage in the codebase to understand patterns
rg 'clip-path' packages/raystack/components/sidebar/ -A 2 -B 2Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Check stylelint configuration
cat .stylelintrc.jsonRepository: raystack/apsara
Length of output: 1065
🏁 Script executed:
# Search for any documentation or comments about clip property replacement
rg -i 'visually.hidden|screen.reader|a11y' packages/raystack/components/sidebar/ -B 3 -A 3Repository: raystack/apsara
Length of output: 776
🌐 Web query:
CSS clip property deprecated clip-path inset replacement modern visually hidden
💡 Result:
- The legacy
clipproperty is deprecated; MDN explicitly recommends usingclip-pathinstead. [1] - The modern equivalent of
clip: rect(...)is typicallyclip-path: inset(...)(e.g.,inset(50%)for “clip everything to nothing”). [2]
Modern “visually hidden (screen-reader only)” pattern (no clip):
.visually-hidden {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip-path: inset(50%);
white-space: nowrap;
border: 0;
}This is the direction major codebases have taken as well (e.g., WordPress removed clip from .screen-reader-text as part of modernizing the pattern). [3]
Sources: [1] (developer.mozilla.org) [2] (developer.mozilla.org) [3] (make.wordpress.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/clip?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/clip-path?utm_source=openai
- 3: https://make.wordpress.org/core/2025/03/20/changes-to-the-screen-reader-text-class-in-wordpress-6-8/?utm_source=openai
Replace deprecated clip with clip-path for modern visually-hidden pattern.
The clip property at line 133 is deprecated and triggers stylelint's property-no-deprecated rule (inherited from stylelint-config-standard-scss). Replace it with the modern clip-path: inset(50%); equivalent, which is the standard approach for screen-reader-only content across major projects.
🛠️ Proposed fix
.root[data-closed] .nav-text {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
- clip: rect(0, 0, 0, 0);
+ clip-path: inset(50%);
white-space: nowrap;
border: 0;
}📝 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.
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip: rect(0, 0, 0, 0); | |
| white-space: nowrap; | |
| border: 0; | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip-path: inset(50%); | |
| white-space: nowrap; | |
| border: 0; |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 133-133: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/sidebar/sidebar.module.css` around lines 127 -
135, Replace the deprecated clip declaration in the CSS rule that currently
contains "clip: rect(0, 0, 0, 0);" with the modern screen-reader-only pattern by
using "clip-path: inset(50%);" (keep the other properties like position, width,
height, padding, margin, overflow, white-space, and border intact); locate the
rule by searching for the declaration "clip: rect(0, 0, 0, 0);" and swap it to
"clip-path: inset(50%);" to satisfy stylelint's property-no-deprecated rule.
Issue-577
Issue-600
Description
prefers-reduced-motion: reducemedia query to tell browser to not use transitions for these settings enabled in browser.Sidebar docs before:

Sidebar docs after:

Type of Change
How Has This Been Tested?
Manual and unit tests
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
-
- Preserve layout and spacing when collapsing the sidebar; collapsed labels remain accessible in the DOM.
-
- Respects reduced-motion settings; collapse/hover animations disabled when requested. Footer/list semantics improved for assistive tech.
-
- Compact sidebar alignment and sizing updates; improved collapsed spacing and hover behaviors.
-
- Demo preview layout options and added icons for richer sidebar examples; preview styling hook added.
-
- Expanded Sidebar docs: Main, Footer, collapse/tooltip behaviors, and reduced-motion guidance.
-
- Updated tests to reflect accessibility and collapsed-state behavior changes.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *Bug Fixes
Accessibility
Style
New Features
Documentation
Tests