feat(web): add dashboard placeholder#49
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a Shadcn/Tailwind design system, many new UI components (sidebar, sheet, tooltip, avatar, etc.), theme CSS and a Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Router as Router
participant AppShell as AppShell
participant SidebarProvider as SidebarProvider
participant DashboardPage as DashboardPage
participant ScrollArea as ScrollArea
Browser->>Router: Navigate to /dashboard
Router->>AppShell: render route
AppShell->>SidebarProvider: initialize (cookie, shortcut, openMobile)
AppShell->>DashboardPage: mount dashboard
DashboardPage->>ScrollArea: mount scroll viewport
Browser->>ScrollArea: user scrolls
ScrollArea-->>SidebarProvider: overflow state change
SidebarProvider-->>DashboardPage: update overflow shadow / UI state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/package.json`:
- Line 27: Move the "shadcn" dependency out of runtime dependencies into
devDependencies in package.json: remove the "shadcn": "^4.5.0" entry from the
dependencies block and add it under devDependencies instead, ensuring the
package is only installed for development/codegen use; after updating, run your
package manager (npm/yarn/pnpm install) to update the lockfile and verify no
runtime imports reference shadcn in the codebase (so the change is safe).
In `@apps/web/src/App.tsx`:
- Around line 37-46: The AppShell uses a brittle exact match (path ===
'/dashboard') which fails for nested or trailing-slash dashboard routes; update
the layout decision in AppShell so it checks the route prefix (e.g.,
path.startsWith('/dashboard')) or, preferably, use route metadata on the router
to mark dashboard routes and read that metadata in AppShell (look for AppShell,
useLocation, path, RouteView, router, and Layout) to decide whether to return
route directly or wrap it with Layout.
In `@apps/web/src/components/ui/button.tsx`:
- Line 61: The button's className is being passed into the cva call via
buttonVariants({ variant, size, className }); update the composition to match
siblings by removing className from the cva invocation and instead call
cn(buttonVariants({ variant, size }), className) so the final className is
composed as cn(buttonVariants(...), className); update the JSX where className
is set (the cn(...) call) in button.tsx to use this pattern referencing
buttonVariants, variant, size, and className.
In `@apps/web/src/components/ui/separator.tsx`:
- Around line 19-22: The separator's sizing selectors use incorrect
data-horizontal/data-vertical tokens so Tailwind rules never apply; in the
Separator component update the className construction (where cn is used) to
target Radix's data-orientation attribute instead, e.g. replace
data-horizontal:... and data-vertical:... with attribute selectors
[data-orientation="horizontal"]:... and [data-orientation="vertical"]:...
(preserve the same h-px/w-full/w-px/self-stretch utility classes) so the
Separator renders correct width/height while still merging the incoming
className prop.
In `@apps/web/src/components/ui/sidebar.tsx`:
- Around line 181-205: The mobile branch currently spreads {...props} onto
<Sheet> which forwards to Radix and drops/ignores typical div attributes; move
the {...props} spread from <Sheet> to the inner visible container (the <div
className="flex h-full w-full flex-col"> inside <SheetContent>) so consumer
attributes like id, style and aria-* apply to the mobile sidebar surface just
like the desktop branch does; ensure you remove {...props} from the <Sheet>
element and add it to that inner div (keeping children placed unchanged).
In `@apps/web/src/components/ui/tooltip.tsx`:
- Around line 44-47: Update the tooltip content class list to remove the invalid
`data-open:*` classes and instead apply the same animation classes to the Radix
`data-state="instant-open"` state; specifically, in the component that builds
the className (the Tooltip content JSX in tooltip.tsx), delete
`data-open:animate-in data-open:fade-in-0 data-open:zoom-in-95` and add
`data-[state=instant-open]:animate-in data-[state=instant-open]:fade-in-0
data-[state=instant-open]:zoom-in-95` so both `data-[state=delayed-open]` and
`data-[state=instant-open]` use the same animation keyframes and the
`data-open:` classes are no longer relied upon.
In `@apps/web/src/hooks/use-mobile.ts`:
- Around line 5-17: The useIsMobile hook recreates the subscribe and getSnapshot
functions (and matchMedia allocation) on every render causing unnecessary
remove/add eventListener churn; fix by hoisting/memoizing the stable subscribe
and getSnapshot callbacks and the media query MediaQueryList: create the query
string once, create a single MediaQueryList via window.matchMedia(query) stored
in a stable ref or module-scoped variable, implement a stable subscribe function
that adds/removes listeners using that MQL, and implement a stable getSnapshot
that returns mql.matches, then pass those stable functions to
useSyncExternalStore inside useIsMobile so React no longer resubscribes each
render.
In `@apps/web/src/index.css`:
- Around line 218-250: The dark theme CSS in the .dark block maps core tokens to
the shadcn defaults (e.g., --primary: oklch(0.922 0 0) and --sidebar-primary:
oklch(0.488 0.243 264.376)), which overrides our Reda brand colors in dark mode;
update the .dark token mappings so --primary, --primary-foreground,
--sidebar-primary, --sidebar-primary-foreground (and any brand-related tokens
like --accent/--accent-foreground or --muted/--muted-foreground) use the Reda
forest/gold palette equivalents instead of the defaults, ensuring color values
match the brand tokens used in light mode (copy or transform the brand token
values into the .dark block and verify contrast for accessibility).
- Around line 35-43: The body rule unconditionally applying bg-background and
text-foreground overrides your brand styles; either scope those shadcn utility
tokens to only the dashboard surface (e.g., replace the global body selector use
of bg-background/text-foreground with a dashboard-specific selector such as
.dashboard or [data-dashboard] so only the dashboard pages get the shadcn
neutrals), or instead update the :root CSS variables --background and
--foreground to match the existing brand values so the global body/html rules
can remain and shadcn utilities inherit the brand tokens; locate the existing
body, html and :root rules in this file and apply one of these two approaches
consistently.
In `@apps/web/src/pages/DashboardPage.tsx`:
- Around line 186-189: The sidebar and language toggle anchors (`<a
href="/dashboard">` inside the navItems rendering and the language links at
lines noted) all point to a static URL causing full page reloads; update the
component to use client-side navigation or make the items inert: replace the
plain anchor with your router-aware Link (e.g., Next.js `Link`) or wrap the
content in a button/element with an onClick that calls router.push(target) (or
e.preventDefault() if keeping `<a>`), and for placeholders make them
non-navigating by removing the href and adding `role="button"`/`tabIndex={0}`
and an onClick that does nothing or opens the intended UI; ensure you update the
elements that render `label` and `Icon` inside the navItems and the language
toggles so they no longer cause full reloads.
- Around line 159-165: The scroll handler currently reads event.target which can
be any nested scrollable; change handleNavigationScroll to read the Radix
ScrollArea viewport's scrollTop instead of event.target: use event.currentTarget
(or query the viewport element from it, e.g. (event.currentTarget as
HTMLElement).querySelector('[data-radix-scroll-area-viewport]')?.scrollTop) and
pass that value into setShowOverflowShadow so only the ScrollArea viewport
controls the overflow shadow; keep the handler signature handleNavigationScroll
and update the call site to remain onScrollCapture if desired.
- Around line 287-295: In the DashboardPage skeleton maps (the Array.from({
length: 4 }).map(...) that renders team-activity rows) and the metric preview
maps, replace key={index} with a stable, explicit key to avoid future
reconciliation bugs — for example map over a fixed list of ids (e.g.,
['s1','s2','s3','s4']) or use a constant namespaced id like
"team-activity-skeleton-<id>" so each Skeleton/SkeletonLine has a stable unique
key; update the map callbacks in DashboardPage.tsx and the metric preview map
usages (the metric preview rendering blocks) to use those stable keys instead of
the numeric index.
- Line 19: Move the file logo.svg into the local assets folder used by the app
(apps/web/src/assets/) and update the imports that reference the deep relative
path: replace the '../../../../logo.svg' import in the DashboardPage component
(import statement near the top of DashboardPage.tsx) with '../assets/logo.svg',
and likewise update the import in the test file referenced in
DashboardPage.vitest.tsx (the import at the top of that file). Ensure the asset
filename matches existing assets (case-sensitive) and run the tests/build to
verify the new path resolves.
In `@apps/web/src/pages/DashboardPage.vitest.tsx`:
- Around line 68-81: The test in the "fades the sidebar overflow shadow after
navigation scroll starts" case queries the viewport with
container.querySelector('[data-slot="scroll-area-viewport"]') and then unsafely
casts it with `as Element`; replace that cast with a proper null guard: assign
the result to a const (e.g., `const viewport = container.querySelector(...)`),
assert it exists with something like `expect(viewport).not.toBeNull()` or
throw/return early if null, and only then call fireEvent.scroll(viewport as
Element, ...). This keeps the test failure focused (locating a missing slot) and
avoids the misleading "target is null" runtime error when invoking
fireEvent.scroll on a null value; reference the DashboardPage render and the
queried selector '[data-slot="scroll-area-viewport"]' when making the change.
- Around line 47-66: The test "keeps dashboard navigation icons on the right
with constrained labels" asserts Tailwind v3-style class substrings that the
DashboardPage component doesn't produce; update the assertions in
apps/web/src/pages/DashboardPage.vitest.tsx to match the actual Tailwind v4
output from DashboardPage: replace checks for
"[&_[data-slot=scroll-area-scrollbar]]:left-0", "...:right-auto", and
"[&_[data-slot=scroll-area-viewport]]:pl-3" with the v4 universal-descendant
forms the component emits (e.g., look for strings containing
":data-[slot=scroll-area-scrollbar]:left-0" and
":data-[slot=scroll-area-viewport]:pl-3" or just assert the key parts like
"data-[slot=scroll-area-scrollbar]" and "left-0"); remove the incorrect shadow
expectation and instead assert the actual overflowShadow classes the component
renders (e.g., "pointer-events-none", "absolute", "inset-x-0", "bottom-0",
"h-14", "bg-linear-to-t", "data-[visible=true]:opacity-100" or
"transition-opacity"); and change the aiLabel word-wrap assertion from
"break-words" to the Tailwind v4 class used in DashboardPage
("wrap-break-word"). Use the existing selectors (aiLabel, navLink, scrollArea,
overflowShadow) to locate elements when updating these expectations.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3757f3fd-ffed-4040-8525-879c0ea60579
⛔ Files ignored due to path filters (2)
apps/web/src/assets/dashboard/dashboard-mural.pngis excluded by!**/*.pnglogo.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
apps/web/components.jsonapps/web/package.jsonapps/web/src/App.tsxapps/web/src/components/ui/avatar.tsxapps/web/src/components/ui/badge.tsxapps/web/src/components/ui/button.tsxapps/web/src/components/ui/card.tsxapps/web/src/components/ui/input.tsxapps/web/src/components/ui/scroll-area.tsxapps/web/src/components/ui/separator.tsxapps/web/src/components/ui/sheet.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/components/ui/skeleton.tsxapps/web/src/components/ui/tooltip.tsxapps/web/src/hooks/use-mobile.tsapps/web/src/index.cssapps/web/src/lib/utils.tsapps/web/src/pages/ContactPage.tsxapps/web/src/pages/DashboardPage.tsxapps/web/src/pages/DashboardPage.vitest.tsxapps/web/src/pages/TrialPage.tsxapps/web/tsconfig.jsonapps/web/vite.config.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the
@cs/*path aliases fromtsconfig.base.jsonfor cross-package imports in TypeScript files
Files:
apps/web/vite.config.tsapps/web/src/lib/utils.tsapps/web/src/pages/ContactPage.tsxapps/web/src/hooks/use-mobile.tsapps/web/src/components/ui/tooltip.tsxapps/web/src/components/ui/input.tsxapps/web/src/components/ui/skeleton.tsxapps/web/src/components/ui/separator.tsxapps/web/src/App.tsxapps/web/src/components/ui/badge.tsxapps/web/src/components/ui/sheet.tsxapps/web/src/pages/TrialPage.tsxapps/web/src/pages/DashboardPage.vitest.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/components/ui/avatar.tsxapps/web/src/components/ui/card.tsxapps/web/src/components/ui/button.tsxapps/web/src/pages/DashboardPage.tsxapps/web/src/components/ui/scroll-area.tsx
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: For core logic.tsfiles, do not add a new responsibility to an existing file when it should be a sibling module
No new core logic.tsfile may exceed 240 LOC without an explicit entry inmodularity-policy.json
If a core logic file already exceeds 240 LOC, it must not grow unless itsmodularity-policy.jsonentry is intentionally updated with rationale
Files classified asmust_splitinmodularity-policy.jsonare debt containers: patches are allowed, but adding unrelated responsibilities is not
Files:
apps/web/vite.config.tsapps/web/src/lib/utils.tsapps/web/src/hooks/use-mobile.ts
🧠 Learnings (4)
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.{ts,tsx} : Use the `cs/*` path aliases from `tsconfig.base.json` for cross-package imports in TypeScript files
Applied to files:
apps/web/vite.config.tsapps/web/tsconfig.json
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.ts : For core logic `.ts` files, do not add a new responsibility to an existing file when it should be a sibling module
Applied to files:
apps/web/tsconfig.json
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.ts : No new core logic `.ts` file may exceed 240 LOC without an explicit entry in `modularity-policy.json`
Applied to files:
apps/web/tsconfig.json
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.ts : Files classified as `must_split` in `modularity-policy.json` are debt containers: patches are allowed, but adding unrelated responsibilities is not
Applied to files:
apps/web/tsconfig.json
🪛 Biome (2.4.12)
apps/web/src/index.css
[error] 5-5: Tailwind-specific syntax is disabled.
(parse)
[error] 36-36: Tailwind-specific syntax is disabled.
(parse)
[error] 39-39: Tailwind-specific syntax is disabled.
(parse)
[error] 42-42: Tailwind-specific syntax is disabled.
(parse)
[error] 140-181: Tailwind-specific syntax is disabled.
(parse)
🪛 Stylelint (17.9.0)
apps/web/src/index.css
[error] 5-5: Unexpected unknown at-rule "@custom-variant" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 140-140: Unexpected unknown at-rule "@theme" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🔇 Additional comments (9)
apps/web/tsconfig.json (1)
13-16: LGTM!The
@/*alias mirrors the Vite alias inapps/web/vite.config.ts, so TS resolution and bundling stay aligned. Intra-package usage of@/*does not conflict with the cross-package@cs/*convention.apps/web/vite.config.ts (1)
12-16: LGTM!The
@alias resolves to the same target as the new@/*TS path mapping, so dev server, build, and Vitest will share consistent module resolution.apps/web/src/pages/ContactPage.tsx (1)
110-110: LGTM — equivalent in Tailwind v4.
bg-white/3produces 3% opacity, matching the previousbg-white/[0.03]value, and is the canonical (non-arbitrary) form in v4.apps/web/src/pages/TrialPage.tsx (1)
56-60: LGTM — Tailwind v4 utility migrations are correct.
max-w-{n}/max-h-{n}resolve via the v4 spacing scale (n × --spacing, default 0.25rem), andbg-linear-to-ris the v4 rename ofbg-gradient-to-r. All values are valid utilities in Tailwind v4.Confirm Tailwind v4 is actually configured for this app (the package.json keeps
tailwindcss ^4.0.0, so this should be fine, but please double-check the CSS entry uses@import "tailwindcss"rather than v3-style directives, since bothbg-linear-to-randmax-w-200rely on v4 semantics).Also applies to: 135-135
apps/web/components.json (1)
1-25: LGTM — config is consistent with shadcn radix-nova preset and project aliases.
style: "radix-nova",tailwind.config: ""(correct for TW v4),cssVariables: true, andrtl: truealign with the shadcn v4 setup. Allaliasesresolve through the@/*path mapping you added intsconfig.jsonandvite.config.ts, so futurenpx shadcn add ...calls will land files in the expected locations.apps/web/src/lib/utils.ts (1)
1-6: LGTM!Standard
cnhelper combiningclsxwithtailwind-mergefor conflict-aware Tailwind class composition.apps/web/src/App.tsx (1)
8-12: LGTM on the routing/provider wiring.Adding
TooltipProviderat the app root is the right place for shadcn tooltip primitives, and the new/dashboardroute is registered consistently with the existing routes. Just keep the AppShell predicate concern above in mind.Also applies to: 29-33
apps/web/package.json (1)
23-23: Correct the lucide-react version reference.
lucide-react@^1.11.0is a valid, published release (April 2026) and will not fail installation. The breaking changes from v0.470.0 → v1.11.0 are real but the brand icons concern doesn't apply to this codebase—none of the exported icons (ArrowLeft, ArrowRight, BarChart3, Bot, CheckCircle2, Clock, Frown, Image, Mail, MapPin, MessageCircle, MessageSquareOff, Search, TrendingUp, UserCircle, Zap) are brand icons.However, verify that the v1 default
aria-hidden="true"on all icons doesn't regress accessibility for icons used in ContactPage.tsx and TrialPage.tsx. If any icons need labels or are purely decorative, confirm the wrapper or usage sites handle this correctly.> Likely an incorrect or invalid review comment.apps/web/src/components/ui/sidebar.tsx (1)
583-619: This concern is not applicable to a Vite SPA.
apps/webis a pure client-side Vite application with no server-side rendering. The"use client"directive at the top of the file has no effect in Vite and does not indicate SSR support. Consequently,Math.random()in theuseStateinitializer does not create a hydration mismatch — there is only client-side rendering, so server and client values are identical.The suggested mitigation is unnecessary.
> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/pages/DashboardPage.tsx`:
- Line 168: The Sidebar uses a hardcoded calc height h-[calc(100svh-82px)] while
the header uses h-20.5, causing them to drift if the header size changes;
instead introduce a shared CSS variable (e.g. --header-height) in global/theme
CSS, replace the header's h-20.5 with a style/class that references
var(--header-height) and update the Sidebar component (the Sidebar element using
h-[calc(100svh-82px)]) and the SidebarInset padding (pt-...) to use calc(100svh
- var(--header-height)) and var(--header-height) respectively so both elements
remain in sync.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fb853c6-2519-49a6-84f0-09227b114ed3
📒 Files selected for processing (4)
apps/web/package.jsonapps/web/src/components/ui/button.tsxapps/web/src/pages/DashboardPage.tsxapps/web/src/pages/DashboardPage.vitest.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the
@cs/*path aliases fromtsconfig.base.jsonfor cross-package imports in TypeScript files
Files:
apps/web/src/pages/DashboardPage.vitest.tsxapps/web/src/components/ui/button.tsxapps/web/src/pages/DashboardPage.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.{ts,tsx} : Use the `cs/*` path aliases from `tsconfig.base.json` for cross-package imports in TypeScript files
Applied to files:
apps/web/src/pages/DashboardPage.tsx
🔇 Additional comments (7)
apps/web/src/pages/DashboardPage.tsx (2)
159-165: Scroll handler correctly scoped to the Radix viewport.The
target.dataset.slot === 'scroll-area-viewport'guard now prevents stray bubbling scroll events from nested portals/scrollables flipping the overflow-shadow state. Matches thedata-slotemitted byScrollAreaPrimitive.Viewportinapps/web/src/components/ui/scroll-area.tsx.
287-295: Stable skeleton keys applied — LGTM.The team-activity and metric-preview maps now derive stable string keys (
team-skeleton-${i},catalog-skeleton-${i},chart-skeleton-${i},chat-skeleton-${i}) instead of array indices, addressing the prior concern.apps/web/src/pages/DashboardPage.vitest.tsx (2)
55-64: Test assertions now align with the Tailwind v4 classes the component emits.Substring checks updated to the v4 universal-descendant form (
**:data-[slot=...]) and towrap-break-word, and the spuriousshadow-[…]expectation was removed. Tests should now correctly reflect whatDashboardPage.tsxrenders.
70-76: Viewport null guard added — LGTM.
expect(viewport).not.toBeNull()plus the non-null assertion onfireEvent.scrollproduces a clearer failure if the Radix slot name ever changes, instead of an opaque "target is null" runtime error.apps/web/package.json (2)
31-31: Past feedback addressed —shadcncorrectly moved todevDependencies.Thanks for relocating the codegen CLI out of runtime
dependencies. As a minor nit, the rest ofdevDependenciesis alphabetically ordered; consider movingshadcndown betweenreact-related entries andtailwindcss(i.e., afterreact-plugin-react-hooks/globalsblock) to keep ordering consistent. Not blocking.
23-23: Version1.11.0is the current latest published version on npm — no change needed.The npm registry shows
lucide-react@1.11.0is the latest release and exists in the registry. The version specification^1.11.0is valid and will resolve correctly. No change to the package.json line is required.If this PR includes
lucide-reacticon usage, verify that all imports/usages are compatible with the v1 API (the v0→v1 migration involves API changes, including some renamed/removed icon exports). However, the dependency version itself is correctly specified.> Likely an incorrect or invalid review comment.apps/web/src/components/ui/button.tsx (1)
1-67: LGTM — standard shadcnButtoncorrectly implemented.
cvavariants/sizes properly typed viaVariantProps<typeof buttonVariants>and composed throughcn(buttonVariants({ variant, size }), className).Slot.Rootfromradix-uiis the correct entry point for theasChildpattern.@/lib/utilsimport is intra-package; the@cs/*cross-package guideline does not apply here.The
defaultvariant's[a]:hover:bg-primary/80style gates hover only to anchor elements (viaasChildwith a link). Plain<button>elements using the default variant would lack hover background styling. However, verification shows the default variant is not currently used in the codebase—all Button usages explicitly specify non-default variants (ghost, etc.), so this is not a practical concern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/App.vitest.tsx`:
- Around line 37-57: The tests use expect(screen.getByTestId(...)) and
expect(screen.getByText(...)).toBeDefined(), which is a no-op because getBy*
already throws; update the assertions in the "wraps non-dashboard routes in
marketing layout" and "does not wrap nested dashboard paths in marketing layout"
tests to use expect(...).toBeInTheDocument() (ensure `@testing-library/jest-dom`
is available) or simply rely on the getBy* call without wrapping it in
toBeDefined(); keep the existing expect(screen.queryByTestId(...)).toBeNull()
as-is for the negative case.
In `@apps/web/src/components/ui/separator.vitest.tsx`:
- Around line 5-18: The test for Separator is brittle because it string-matches
the element class list; update the test for the Separator component to assert
meaningful behavior instead: render Separator (using the same data-testid) with
both orientation="vertical" and orientation="horizontal" and assert the DOM
forwards the data-orientation attribute value
(getAttribute('data-orientation')), assert data-slot is forwarded when provided,
assert the decorative prop defaults to true by checking the presence/absence of
the decorative-related attribute or behavior exposed by the component, and
assert that a user-supplied className is merged (e.g., render with
className="foo" and check getAttribute('class') contains "foo"); remove fragile
substring checks for specific Tailwind class tokens and avoid relying on exact
class ordering.
In `@apps/web/src/index.css`:
- Around line 203-207: The chart color tokens --chart-1 through --chart-5 are
defined as neutral oklch grays in :root and re-declared identically in .dark,
causing charts to stay grayscale and the dark redeclaration to be a no-op;
update these tokens to brand-aligned color values (e.g., the Reda forest/gold
palette) in :root so chart series use brand colors, and remove or adjust the
identical declarations inside .dark so dark mode either inherits the brand
values or provides appropriate dark-mode variants (target the declarations for
--chart-1, --chart-2, --chart-3, --chart-4, --chart-5 and the :root and .dark
blocks).
In `@apps/web/src/pages/DashboardPage.tsx`:
- Line 222: The decorative <img> elements in DashboardPage (e.g., the img with
src={logoUrl} and the other decorative images around the "رضا" wordmark and
dashboard mural) should explicitly be hidden from assistive tech; update those
<img> elements (search for the img with className="size-12 object-contain" and
the images around lines 253-257) to include aria-hidden="true" in addition to
the existing alt="" so screen readers will reliably ignore them.
In `@scripts/dev-session-log.ts`:
- Around line 86-104: The code treats any non-empty input.extraArgs as
user-supplied scope, dropping DEFAULT_DEV_FILTERS; change the devScope
calculation to only skip/apply defaults when the user actually provided a
--filter flag. Detect a filter via something like input.extraArgs.some(a =>
a.startsWith("--filter")) (or equivalently matching "--filter" and
"--filter=..."), then set devScope = hasFilter ? input.extraArgs :
[...DEFAULT_DEV_FILTERS, ...input.extraArgs]; update the variable devScope (used
where command args are built) accordingly; if current semantics were
intentional, add a comment on DEFAULT_DEV_FILTERS documenting the behavior
instead.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dc84586-e7a3-46dd-bb16-57b45db4777a
⛔ Files ignored due to path filters (1)
apps/web/src/assets/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
apps/web/src/App.tsxapps/web/src/App.vitest.tsxapps/web/src/components/ui/separator.tsxapps/web/src/components/ui/separator.vitest.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/components/ui/tooltip.tsxapps/web/src/hooks/use-mobile.tsapps/web/src/index.cssapps/web/src/pages/DashboardPage.tsxapps/web/src/pages/DashboardPage.vitest.tsxscripts/dev-session-log.test.tsscripts/dev-session-log.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the
@cs/*path aliases fromtsconfig.base.jsonfor cross-package imports in TypeScript files
Files:
scripts/dev-session-log.test.tsapps/web/src/components/ui/separator.vitest.tsxapps/web/src/hooks/use-mobile.tsapps/web/src/components/ui/separator.tsxscripts/dev-session-log.tsapps/web/src/App.tsxapps/web/src/App.vitest.tsxapps/web/src/pages/DashboardPage.vitest.tsxapps/web/src/components/ui/sidebar.tsxapps/web/src/pages/DashboardPage.tsxapps/web/src/components/ui/tooltip.tsx
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: For core logic.tsfiles, do not add a new responsibility to an existing file when it should be a sibling module
No new core logic.tsfile may exceed 240 LOC without an explicit entry inmodularity-policy.json
If a core logic file already exceeds 240 LOC, it must not grow unless itsmodularity-policy.jsonentry is intentionally updated with rationale
Files classified asmust_splitinmodularity-policy.jsonare debt containers: patches are allowed, but adding unrelated responsibilities is not
Files:
scripts/dev-session-log.test.tsapps/web/src/hooks/use-mobile.tsscripts/dev-session-log.ts
🧠 Learnings (1)
📚 Learning: 2026-04-24T16:14:31.547Z
Learnt from: CR
Repo: HusseinBaraja/CS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T16:14:31.547Z
Learning: Applies to **/*.{ts,tsx} : Use the `cs/*` path aliases from `tsconfig.base.json` for cross-package imports in TypeScript files
Applied to files:
apps/web/src/pages/DashboardPage.tsx
🪛 Biome (2.4.13)
apps/web/src/index.css
[error] 5-5: Tailwind-specific syntax is disabled.
(parse)
[error] 36-36: Tailwind-specific syntax is disabled.
(parse)
[error] 39-39: Tailwind-specific syntax is disabled.
(parse)
[error] 42-42: Tailwind-specific syntax is disabled.
(parse)
[error] 140-181: Tailwind-specific syntax is disabled.
(parse)
🪛 Stylelint (17.9.0)
apps/web/src/index.css
[error] 5-5: Unexpected unknown at-rule "@custom-variant" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 140-140: Unexpected unknown at-rule "@theme" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🔇 Additional comments (8)
apps/web/src/components/ui/separator.tsx (1)
1-28: LGTM — pastdata-orientationfix correctly applied.The selectors on Line 20 now correctly target Radix's
data-orientationattribute, matching the standard shadcn pattern.Optional: the
"use client"directive on Line 1 is a Next.js App Router construct and is a no-op in a Vite app. It's harmless (shadcn boilerplate) and useful if these components are ever copied into a Next.js project, so leaving it is fine.scripts/dev-session-log.test.ts (1)
30-48: Test correctly pins the new default-filter behavior.The added case cleanly asserts the default
--filterfan-out limited to runtime apps whenextraArgsis empty. Behavioral concern about how the default is selected is raised ondev-session-log.ts.apps/web/src/hooks/use-mobile.ts (1)
1-17: LGTM!The previous feedback to hoist
subscribe/getSnapshot/getServerSnapshotto module scope has been correctly applied;useSyncExternalStorenow receives stable references and avoids resubscribing on each render.apps/web/src/components/ui/tooltip.tsx (1)
44-47: LGTM!The previous concern with the invalid
data-open:Tailwind classes has been resolved — bothdata-[state=delayed-open]anddata-[state=instant-open]now share the open animation classes, anddata-[state=closed]drives the close animation, matching Radix Tooltip's actualdata-statevalues.apps/web/src/components/ui/sidebar.tsx (1)
181-207: LGTM on the mobile{...props}fix.Spreading
{...props}onto the inner<div className="flex h-full w-full flex-col">(Line 201) instead of<Sheet>resolves the previous concern — consumer-suppliedid/style/aria-*attributes now reach a real DOM element on mobile, mirroring the desktop branch behavior.apps/web/src/App.tsx (1)
37-46: LGTM on the prefix check.The
path === '/dashboard' || path.startsWith('/dashboard/')guard correctly handles both the exact route and nested dashboard paths, addressing the prior concern about exact-match brittleness.apps/web/src/pages/DashboardPage.tsx (1)
1-317: Past review feedback fully integrated.Several previously raised concerns are cleanly addressed:
- The scroll handler now narrows to the Radix viewport via
target.dataset.slot === 'scroll-area-viewport'(Lines 159–165), avoiding mis-fires from nested scrollables.- Sidebar nav and language-toggle anchors are inert placeholders (
href="#"+event.preventDefault()+data-placeholder="true"), so clicking them no longer triggers a full reload.- Skeleton arrays use stable namespaced keys (e.g.
team-skeleton-${i}) rather than array indices.- The header/sidebar/inset all derive offsets from a shared
--header-heightCSS variable, keeping the layout in sync.apps/web/src/pages/DashboardPage.vitest.tsx (1)
1-107: LGTM on the test alignment fixes.Class-string assertions now match the Tailwind v4 utilities the component actually emits (e.g.
**:data-[slot=scroll-area-viewport]:pl-3,wrap-break-word), and the viewport query is null-guarded beforefireEvent.scroll. Coverage of the RTL shell, logo, nav constraints, placeholder state, scroll-driven shadow, and--header-heightsync is solid. (SametoBeDefined()-on-getBy*nit asApp.vitest.tsxapplies here, but it's harmless.)
1 participant
Summary by CodeRabbit
New Features
Style
Tests