Merged
Conversation
- added the shadcn sidebar component - integrated it with the layout of the current dashboard - extracted a separated layout for auth pages - updated components on the dashboard and project pages to make everything responsive accounting the addition of the sidebar - removed the `main-layout` class from the `index.css` and updated the main layout.tsx component by removing the useEffect using that class
- added the `navigation-reset-wrapper.tsx` component that brings the user back to the top of a page when navigating - added the title in the dependencies array of the useEffect in the `use-page-title` hook
- added only one root tooltip provider in the App.tsx component
- fixed the responsiveness issues in the circle-packing chart - reset the height and width of the svg wrapper when the group check box is toggled to bring the bubbles in the center
- added a useEffect to reset the page of files when a language is selected in the LanguagesDropDown component or when the period/custom range changes
- added a new component called `link-with-query` that wraps the react-router link and adds the search params to the path navigated to - updated affected components : `app-sidebar`, `auth-dropdown` (removed the `AUTH_DROPDOWN_ITEMS` constant), `navbar`, `title`, `period-projects`
- fixed the naming conflict between the google login token and the password reset token by renaming the reset-password token to `reset-password-token` instead of just `token`. This conflict lead to the page reset-password page to be stuck in an infinite loop trying to navigate to the dashboard when the reset password procedure is over - increased the rate limits to 600 requests for 15 minutes
- used the real year instead of hardcoding 2025 in the emails body (`get-onboarding-email-body` and `get-password-reset-email-body`) - prettified the error thrown by the `redirectToVSCodeAfterGoogleAuthLoader` when the zod validation fails
- added csv subsets to the langages subsets: csv (semicolon), csv (pipe), csv (whitespace) and dynamic csv - bumped the extension version to `v0.051`
📝 WalkthroughWalkthroughAdds a shared Sidebar system and UI primitives (Sheet, Separator, Collapsible), restructures dashboard layout/providers (AppSidebar, SidebarProvider, Wrapper), preserves query strings via LinkWithQuery, injects dynamic current year into email templates, increases TRPC default rate limit, and applies multiple responsive/UI and minor loader/auth adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as AppSidebar
participant TRPC as TRPC Client
participant API as Backend API
participant Router as React Router
User->>App: open dashboard / change period / paginate
App->>TRPC: query getPeriodProjects(period, range?, page)
TRPC->>API: fetch projects (period, start, end, page)
API-->>TRPC: projects + hasMore
TRPC-->>App: return data
App->>App: merge projects, update pagination state
User->>App: click project link
App->>Router: navigate via LinkWithQuery (to + current search)
Router-->>User: route change (search params preserved)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/features/files-list/components/languages-dropdown.tsx (1)
29-35:⚠️ Potential issue | 🟡 MinorIcon-only button at narrow viewports lacks an accessible name.
With
max-[20rem]:hidden, Tailwind setsdisplay: noneon the "Languages" span, removing it from the accessibility tree. At viewports ≤ 20rem the<Button>has no visible text and noaria-label, making it unnamed for assistive technologies (violates WCAG 2.1 SC 4.1.2).♿ Proposed fix: add `aria-label` to the button
<Button variant="secondary" className="flex items-center justify-center gap-2" + aria-label="Languages" > <span className="max-[20rem]:hidden">Languages</span> <ChevronDown /> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/features/files-list/components/languages-dropdown.tsx` around lines 29 - 35, The button rendered in the Languages dropdown (the <Button> wrapping the "Languages" <span> and <ChevronDown />) becomes icon-only at narrow viewports because the "Languages" span is hidden; add an accessible name by setting an aria-label (e.g., aria-label="Languages") on that <Button> element so assistive tech still perceives the control when the span is hidden, ensuring the label matches the visible text and remains consistent with the <ChevronDown /> icon.
🧹 Nitpick comments (12)
apps/api/src/trpc/trpc.service.ts (1)
53-53: Consider monitoring for increased abuse risk after doubling the global default.Raising
maxfrom 300 → 600 per 15-minute window doubles the allowed burst for every endpoint that doesn't supply an explicit rate-limiter key/max pair. Since bothpublicProcedureandprotectedProcedurefall through to this default, the change broadens the attack surface at the same IP granularity. If you don't already have alerting onTOO_MANY_REQUESTSerror rates, now is a good time to add it so you can detect abuse patterns before they saturate downstream resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trpc/trpc.service.ts` at line 53, The default rate limit max was doubled (rateLimiterParams -> max = 600) expanding burst allowances for publicProcedure and protectedProcedure; add observability and safer defaults: revert the implicit default back to the previous conservative value (e.g., 300) in trpc.service.ts or require explicit overrides, and instrument the rate-limit rejection handler (where TOO_MANY_REQUESTS is raised) to emit a metric/alert (counter/timestamp with labels like endpoint/procedure/IP) so ops can create an alert on spikes; update publicProcedure/protectedProcedure callers to pass explicit key/max if they need higher limits.apps/dashboard/src/loaders/auth-loader.ts (1)
102-109:googleAuthLoaderstill uses the old manual error aggregation — inconsistent with the PR's centralisation effort
redirectToVSCodeAfterGoogleAuthLoaderwas updated to useformatZodError, butgoogleAuthLoaderstill concatenates issue messages manually. Note that the error thrown here is swallowed by the enclosing try-catch (line 115), so the formatting difference has no user-visible impact today — but it's an inconsistency worth cleaning up.♻️ Proposed fix
if (!validatedCallBackUrl.success) { - throw new Error( - validatedCallBackUrl.error.issues.reduce( - (acc, curr) => acc + curr.message, - "", - ), - ); + throw new Error(formatZodError(validatedCallBackUrl.error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/loaders/auth-loader.ts` around lines 102 - 109, Replace the manual concatenation of Zod issue messages in googleAuthLoader with the centralized formatter: when validatedCallBackUrl.success is false, throw new Error(formatZodError(validatedCallBackUrl.error)) instead of using issues.reduce(...); ensure the function uses the existing formatZodError helper (import or reference the same symbol used by redirectToVSCodeAfterGoogleAuthLoader) so both loaders share consistent Zod error formatting (note the thrown error is caught by the surrounding try/catch in googleAuthLoader).apps/dashboard/src/features/files-circle-packing-chart/hooks/use-animate-chart.tsx (1)
42-44:useMemois unnecessary overhead for trivially cheap arithmetic.
height * 1e-9andwidth * 1e-9are single multiplications — theuseMemobookkeeping (dep tracking, cache allocation, cache invalidation check) costs more than the computation itself. Plainconstvariables are equivalent and simpler.♻️ Proposed refactor
- const boundaryHeightMargin = useMemo(() => height * 1e-9, [height]); - - const boundaryWidthMargin = useMemo(() => width * 1e-9, [width]); + const boundaryHeightMargin = height * 1e-9; + const boundaryWidthMargin = width * 1e-9;If
useMemois removed from these two,useMemocan be dropped from the import at line 1 if it has no other usages in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/features/files-circle-packing-chart/hooks/use-animate-chart.tsx` around lines 42 - 44, Remove the unnecessary useMemo wrappers around the trivial multiplications: replace the computed values derived in boundaryHeightMargin and boundaryWidthMargin (currently using useMemo with height * 1e-9 and width * 1e-9) with plain const assignments, and then remove useMemo from the import list if it has no other usages in the file; update references to boundaryHeightMargin and boundaryWidthMargin as needed.apps/dashboard/src/features/auth/components/layout.tsx (1)
3-8: Consider renaming the componentAuthLayoutto match its aliased import.This file and
apps/dashboard/src/components/layout/layout.tsxboth export a symbol namedLayout. Because the names collide,main.tsxmust alias it on import (Layout as AuthLayout). Renaming the export here eliminates that alias and makes the intent self-documenting.♻️ Proposed rename
-export const Layout = () => { +export const AuthLayout = () => { return ( <div className="flex-1"> <Outlet /> </div> ); };And in
apps/dashboard/src/main.tsx:-import { Layout as AuthLayout } from "@/features/auth/components/layout"; +import { AuthLayout } from "@/features/auth/components/layout";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/features/auth/components/layout.tsx` around lines 3 - 8, Rename the exported React component from Layout to AuthLayout (change the symbol exported by the component currently declared as Layout to AuthLayout) and update all imports/usages that currently import it (notably the place where it was imported as Layout as AuthLayout) to import AuthLayout directly; specifically, update the component declaration name (Layout -> AuthLayout), its export, and any references/imports in main.tsx and other consumers so there is no aliased import.packages/ui/src/components/ui/sidebar.tsx (1)
80-85: Consider addingSameSiteandSecureattributes to the sidebar state cookie.The cookie is set without
SameSiteorSecureflags. While this is just a UI preference cookie with no security implications, browsers defaultSameSitetoLaxwhich is fine, but being explicit is a good practice. If the dashboard is served over HTTPS,Secureensures the cookie isn't sent over plain HTTP.Optional fix
- document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax; Secure`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ui/sidebar.tsx` around lines 80 - 85, The cookie assignment for SIDEBAR_COOKIE_NAME in the effect using setOpenProp/open should explicitly include SameSite and Secure attributes: append "; SameSite=Lax" (or your chosen policy) to the cookie string and add "; Secure" when the app is served over HTTPS (detect via location.protocol === 'https:' or window.isSecureContext) so the code that sets document.cookie with SIDEBAR_COOKIE_NAME and SIDEBAR_COOKIE_MAX_AGE includes these flags.apps/dashboard/src/components/layout/navigation-reset-wrapper.tsx (1)
4-12: Consider a more descriptive export name thanWrapper.The file is named
navigation-reset-wrapper.tsxbut exportsWrapper, which is very generic. At the import site (App.tsx),Wrapperdoesn't convey what it does. A name likeScrollResetWrapperorNavigationScrollResetwould be self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/navigation-reset-wrapper.tsx` around lines 4 - 12, Rename the generic exported component Wrapper to a descriptive name (e.g., ScrollResetWrapper or NavigationScrollReset) to reflect its behavior; update the component declaration/export (Wrapper) to the new name and change all import sites (for example where App.tsx imports Wrapper) to the new identifier so references remain consistent; keep the existing implementation (useLocation, useLayoutEffect, window.scrollTo) unchanged—only change the component name and its imports/exports to improve clarity.apps/dashboard/src/App.tsx (1)
99-105:SidebarProviderregisters a keyboard shortcut listener (Ctrl/Cmd+B) globally, including on auth pages where it's unused.The route structure confirms auth pages (
login,register,forgot-password, etc.) render through a separate<AuthLayout />, but sinceSidebarProviderwraps the<Outlet />inApp.tsx, the keyboard listener and cookie persistence are active on all routes—including auth pages that don't render a sidebar.While auth pages won't break (the listener simply has no sidebar to toggle), this is unnecessary overhead. Consider moving
SidebarProviderto wrap only the dashboard layout, or conditionally apply it based on the route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/App.tsx` around lines 99 - 105, The SidebarProvider is mounted globally in App.tsx causing its keyboard listener and cookie persistence to run on auth routes; move the SidebarProvider so it only wraps the dashboard routes (e.g., wrap it around the dashboard layout/component that renders the sidebar, such as DashboardLayout) or conditionally render SidebarProvider in App.tsx by checking the current route (exclude paths like login, register, forgot-password) before mounting it; adjust references in App.tsx (remove SidebarProvider around Outlet/Wrapper) and ensure the sidebar-related state/hooks still wrap the components that need them (Wrapper, components inside DashboardLayout) so the global listener is not active on AuthLayout routes.apps/dashboard/src/components/layout/app-sidebar.tsx (3)
96-99: The header<div>withcursor-pointerisn't interactive — consider making it a link or button.The
SidebarMenuButtonwithasChilddelegates rendering to its child, which is a plain<div>. A<div>withcursor-pointeris not keyboard-accessible (no focus/activation). If this is meant to be clickable (e.g., navigate to a user profile or dashboard root), wrap it in a<LinkWithQuery>or<button>. If it's purely decorative, removecursor-pointer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/app-sidebar.tsx` around lines 96 - 99, The container div that currently renders the Logo and {username} is marked cursor-pointer but is not keyboard-accessible because the SidebarMenuButton (with asChild) delegates to a plain <div>; change the child to an interactive element: either wrap the Logo+username in a LinkWithQuery (for navigation) or render a <button> with the click handler used by SidebarMenuButton, and ensure it keeps the same classes (size-6!, group-data-[collapsible=icon]:size-4!) and content; if the element is purely decorative, remove cursor-pointer and any click delegation. Also ensure the chosen element supports focus/keyboard activation and include an accessible label/aria-label if needed.
59-76: Effect ordering is fragile — merge effect runs before reset effect on period change.When
periodchanges, both effects trigger in the same render:
- Line 60 (merge effect): appends new
periodProjectsto the stale list.- Line 73 (reset effect): overwrites
projectsToDisplaywith freshperiodProjects.The reset effect "wins" because it runs second, but the intermediate merge is wasted work and the pattern is brittle if effect order changes.
Additionally,
periodProjectsis read inside the reset effect (line 75) but not listed in its dependency array (line 76). This works under suspense guarantees but will trigger anexhaustive-depslint warning.Consider consolidating both effects into a single effect that distinguishes "new page loaded" from "period/range changed":
♻️ Consolidation sketch
One approach: track whether the period/range changed and branch accordingly in a single effect.
+ // Track previous period + range to detect resets + const periodKey = period === "Custom Range" + ? `${customRange.start}-${customRange.end}` + : period; + const [prevPeriodKey, setPrevPeriodKey] = useState(periodKey); + + useEffect(() => { + if (periodKey !== prevPeriodKey) { + // Period changed → full reset + setPage(1); + setProjectsToDisplay(periodProjects); + setPrevPeriodKey(periodKey); + } else { + // Same period, new page → merge + setProjectsToDisplay((prev) => { + const displayed = new Set(prev.map((p) => p.name)); + return [...prev, ...periodProjects.filter((e) => !displayed.has(e.name))]; + }); + } + }, [periodProjects, periodKey, prevPeriodKey]); - - const [projectsToDisplay, setProjectsToDisplay] = useState(periodProjects); - useEffect(() => { - setProjectsToDisplay((prev) => { - const displayedProjectNames = new Set(prev.map((p) => p.name)); - return [ - ...prev, - ...periodProjects.filter( - (entry) => !displayedProjectNames.has(entry.name), - ), - ]; - }); - }, [periodProjects]); - - // Reset page when period or date range changes - useEffect(() => { - setPage(1); - setProjectsToDisplay(periodProjects); - }, [period, customRange.start, customRange.end]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/app-sidebar.tsx` around lines 59 - 76, The two useEffect hooks that update projectsToDisplay are fragile and do redundant work; replace them with a single useEffect that depends on periodProjects, period, customRange.start, and customRange.end, and inside it branch: when period or customRange changed call setPage(1) and setProjectsToDisplay(periodProjects) (reset path), otherwise merge new entries from periodProjects into the existing projectsToDisplay state (merge path), ensuring you read periodProjects only from the effect deps to satisfy exhaustive-deps and reference the existing state via the functional updater (setProjectsToDisplay(prev => ...)) and preserve the current merge logic that builds a Set of existing names.
41-57: Parallelize two sequentialuseSuspenseQuerycalls to prevent request waterfall.The projects query (line 43) suspends until resolved before the component renders and reaches the user query (line 80), serializing two independent network requests. Use
useSuspenseQueriesto initiate both queries concurrently instead.♻️ Refactor: parallelize with useSuspenseQueries
- const { - data: { periodProjects, hasNext }, - } = useSuspenseQuery( - trpc.analytics.projects.getPeriodProjects.queryOptions( - period === "Custom Range" - ? { - start: customRange.start, - end: customRange.end, - page, - } - : { - start: PERIODS_CONFIG[period].start, - end: PERIODS_CONFIG[period].end, - page, - }, - ), - ); ... - const { - data: { email, username }, - } = useSuspenseQuery(trpc.auth.getUser.queryOptions()); + const [projectsResult, userResult] = useSuspenseQueries({ + queries: [ + trpc.analytics.projects.getPeriodProjects.queryOptions( + period === "Custom Range" + ? { start: customRange.start, end: customRange.end, page } + : { start: PERIODS_CONFIG[period].start, end: PERIODS_CONFIG[period].end, page }, + ), + trpc.auth.getUser.queryOptions(), + ], + }); + + const { periodProjects, hasNext } = projectsResult.data; + const { email, username } = userResult.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/app-sidebar.tsx` around lines 41 - 57, The projects query using useSuspenseQuery (trpc.analytics.projects.getPeriodProjects.queryOptions with PERIODS_CONFIG/customRange and page) is being awaited before the later user query, causing a request waterfall; replace the two sequential useSuspenseQuery calls with a single useSuspenseQueries call that runs both queries in parallel (one entry built from trpc.analytics.projects.getPeriodProjects.queryOptions(...) using period/PERIODS_CONFIG/customRange/page and the other entry for the user query), then destructure the returned array to get periodProjects/hasNext and the user data so the component can render once both resolve concurrently.apps/dashboard/src/features/files-circle-packing-chart/components/circular-packing.tsx (1)
34-45: WrapresetSVGDimensionsinuseCallbackto fix memoization and stale effect reference.Two issues with the current approach:
resetSVGDimensionsis recreated every render, defeating thememoonOptionsSection(it receives a new function reference each time).- The
useEffecton line 39 captures the initial render's closure and never updates the listener (empty deps[]). It works by coincidence here since only stable refs and globals are read, but it will trigger anexhaustive-depslint warning.Both are solved by wrapping in
useCallback:♻️ Proposed fix
-import { RefObject, useEffect, useState } from "react"; +import { RefObject, useCallback, useEffect, useState } from "react";- const resetSVGDimensions = () => { + const resetSVGDimensions = useCallback(() => { setWidth(parentDivRef.current?.clientWidth ?? (window.innerWidth * 5) / 6); setHeight((window.innerWidth * 2) / 3); - }; + }, [parentDivRef]); useEffect(() => { window.addEventListener("resize", resetSVGDimensions); return () => { window.removeEventListener("resize", resetSVGDimensions); }; - }, []); + }, [resetSVGDimensions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/features/files-circle-packing-chart/components/circular-packing.tsx` around lines 34 - 45, The resetSVGDimensions function is recreated each render and causes a stale listener and memo invalidation for OptionsSection; wrap resetSVGDimensions in useCallback (e.g., const resetSVGDimensions = useCallback(() => { ... }, [parentDivRef.current, setWidth, setHeight])) and then keep the useEffect that adds/removes the resize listener but include resetSVGDimensions in its dependency array so the listener always references the memoized callback; ensure the callback reads parentDivRef.current safely and uses setWidth/setHeight from props/state in its deps.apps/dashboard/src/components/layout/layout.tsx (1)
24-33: Consider using<SidebarInset>instead of a plain<div className="flex-1">.Since
AppSidebarusesvariant="inset", the layout should wrap the main content area with<SidebarInset>. This component applies responsive styling via peer-data selectors (margins, rounded corners, shadows) that coordinate with the sidebar's collapsed/expanded states, providing proper spacing and visual cohesion between the sidebar and content area.Import
SidebarInsetfrom@repo/ui/components/ui/sidebarand replace the<div className="flex-1">wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/layout.tsx` around lines 24 - 33, Replace the plain wrapper div around the main content with the SidebarInset component so the sidebar's inset variant and peer-data responsive styling apply: import SidebarInset from `@repo/ui/components/ui/sidebar`, replace the <div className="flex-1"> that currently contains <Header />, the <div id="loader" ...> (with isLoading, GlobalSpinner, Outlet), <ScrollToTopButton /> and <Footer /> with <SidebarInset className="flex-1"> ... </SidebarInset>, preserving the id="loader" inner div and existing children (Header, GlobalSpinner/Outlet, ScrollToTopButton, Footer) and any className logic (cn(isLoading && "opacity-70")).
🤖 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/dashboard/src/components/common/link-with-query.tsx`:
- Around line 4-18: The LinkWithQuery component is overcomplicated and fragile:
remove the useState and useEffect that read window.location.search and instead
read the reactive value from useLocation() directly. Inside LinkWithQuery, call
useLocation() (e.g., const { search } = useLocation()) and use that search when
constructing the Link (to={to + search}); delete the setSearch/useEffect logic
and any references to window.location.search so the component updates
immediately on navigation and avoids stale renders and linter warnings.
In `@apps/dashboard/src/features/files-list/components/files.tsx`:
- Around line 37-39: The languagesToFetch array is recreated each render which
makes the Files component's useEffect (the one that calls setPage(1) with
[period, customRange.start, customRange.end, languagesToFetch]) fire every time;
fix this by memoizing the languagesToFetch computation in the parent FilesList
component using useMemo so it only changes when selectedEntries changes (i.e.,
compute languagesToFetch via useMemo(() => selectedEntries.length ?
selectedEntries.map(e => e.languageSlug) : undefined, [selectedEntries])) so the
Files component receives a stable reference and pagination no longer resets.
In
`@apps/dashboard/src/features/period-languages-chart/components/period-languages-chart.tsx`:
- Around line 58-60: The Tailwind breakpoint max-[18rem] on the <h2> in
period-languages-chart.tsx will never match real viewports; update the heading’s
responsive class (the <h2 className="... max-[18rem]:text-base">) to a practical
breakpoint or use a container query: either replace max-[18rem]:text-base with a
meaningful viewport breakpoint like max-sm:text-base or max-[20rem]:text-base,
or implement a CSS container query (make the chart panel a container and apply
an element-width rule to reduce font-size) so the rule fires when the component
is narrow rather than relying on the tiny viewport breakpoint.
In `@apps/dashboard/src/features/period-projects/components/period-projects.tsx`:
- Line 12: LinkWithQuery uses useState(useLocation().search) and a
window.location.search effect which is fragile; remove the local state and
effect and instead read the current search directly from the router hook: call
useLocation() inside the LinkWithQuery component (e.g., const { search } =
useLocation()) and use that search value when building the link href/target,
preserving any existing prop merging logic; delete the window.location.search
dependency and state initialization (and related setState) so the component
reacts to location changes via useLocation() only.
In `@packages/ui/src/components/ui/sheet.tsx`:
- Around line 72-78: The close button currently uses SheetPrimitive.Close
wrapping Icon (XIcon) which discards the <span className="sr-only">Close</span>,
leaving the button without an accessible name; update the SheetPrimitive.Close
(or the component that renders the actual button) to include an
aria-label="Close" (or aria-label prop driven by showCloseButton) so screen
readers get a name, and remove the sr-only span; also revise the positioning
class on the Icon/SheetPrimitive.Close from "absolute bottom-14" to a top-right
placement (e.g., absolute top-? right-? consistent with your design system) or
confirm and document if bottom placement is intentional. Ensure changes target
the SheetPrimitive.Close / Icon (XIcon) usage so accessibility and layout are
fixed.
In `@packages/ui/src/globals.css`:
- Around line 46-51: The CSS custom properties (--shadow-x, --shadow-y,
--shadow-blur, --shadow-spread, --shadow-opacity, --shadow-color) declared in
:root and .dark are unused; either remove both declarations to avoid dead code
or document them as intentional placeholders and wire them into your shadow
utilities (e.g., replace hardcoded values in shadow-2xs...shadow-2xl with these
variables). Locate the declarations by the exact variable names and update
globals.css accordingly, and if you choose to keep them add a comment above the
declarations explaining they are reserved for future theming or ensure the
Tailwind/shadow utility definitions reference those variables.
---
Outside diff comments:
In `@apps/dashboard/src/features/files-list/components/languages-dropdown.tsx`:
- Around line 29-35: The button rendered in the Languages dropdown (the <Button>
wrapping the "Languages" <span> and <ChevronDown />) becomes icon-only at narrow
viewports because the "Languages" span is hidden; add an accessible name by
setting an aria-label (e.g., aria-label="Languages") on that <Button> element so
assistive tech still perceives the control when the span is hidden, ensuring the
label matches the visible text and remains consistent with the <ChevronDown />
icon.
---
Nitpick comments:
In `@apps/api/src/trpc/trpc.service.ts`:
- Line 53: The default rate limit max was doubled (rateLimiterParams -> max =
600) expanding burst allowances for publicProcedure and protectedProcedure; add
observability and safer defaults: revert the implicit default back to the
previous conservative value (e.g., 300) in trpc.service.ts or require explicit
overrides, and instrument the rate-limit rejection handler (where
TOO_MANY_REQUESTS is raised) to emit a metric/alert (counter/timestamp with
labels like endpoint/procedure/IP) so ops can create an alert on spikes; update
publicProcedure/protectedProcedure callers to pass explicit key/max if they need
higher limits.
In `@apps/dashboard/src/App.tsx`:
- Around line 99-105: The SidebarProvider is mounted globally in App.tsx causing
its keyboard listener and cookie persistence to run on auth routes; move the
SidebarProvider so it only wraps the dashboard routes (e.g., wrap it around the
dashboard layout/component that renders the sidebar, such as DashboardLayout) or
conditionally render SidebarProvider in App.tsx by checking the current route
(exclude paths like login, register, forgot-password) before mounting it; adjust
references in App.tsx (remove SidebarProvider around Outlet/Wrapper) and ensure
the sidebar-related state/hooks still wrap the components that need them
(Wrapper, components inside DashboardLayout) so the global listener is not
active on AuthLayout routes.
In `@apps/dashboard/src/components/layout/app-sidebar.tsx`:
- Around line 96-99: The container div that currently renders the Logo and
{username} is marked cursor-pointer but is not keyboard-accessible because the
SidebarMenuButton (with asChild) delegates to a plain <div>; change the child to
an interactive element: either wrap the Logo+username in a LinkWithQuery (for
navigation) or render a <button> with the click handler used by
SidebarMenuButton, and ensure it keeps the same classes (size-6!,
group-data-[collapsible=icon]:size-4!) and content; if the element is purely
decorative, remove cursor-pointer and any click delegation. Also ensure the
chosen element supports focus/keyboard activation and include an accessible
label/aria-label if needed.
- Around line 59-76: The two useEffect hooks that update projectsToDisplay are
fragile and do redundant work; replace them with a single useEffect that depends
on periodProjects, period, customRange.start, and customRange.end, and inside it
branch: when period or customRange changed call setPage(1) and
setProjectsToDisplay(periodProjects) (reset path), otherwise merge new entries
from periodProjects into the existing projectsToDisplay state (merge path),
ensuring you read periodProjects only from the effect deps to satisfy
exhaustive-deps and reference the existing state via the functional updater
(setProjectsToDisplay(prev => ...)) and preserve the current merge logic that
builds a Set of existing names.
- Around line 41-57: The projects query using useSuspenseQuery
(trpc.analytics.projects.getPeriodProjects.queryOptions with
PERIODS_CONFIG/customRange and page) is being awaited before the later user
query, causing a request waterfall; replace the two sequential useSuspenseQuery
calls with a single useSuspenseQueries call that runs both queries in parallel
(one entry built from
trpc.analytics.projects.getPeriodProjects.queryOptions(...) using
period/PERIODS_CONFIG/customRange/page and the other entry for the user query),
then destructure the returned array to get periodProjects/hasNext and the user
data so the component can render once both resolve concurrently.
In `@apps/dashboard/src/components/layout/layout.tsx`:
- Around line 24-33: Replace the plain wrapper div around the main content with
the SidebarInset component so the sidebar's inset variant and peer-data
responsive styling apply: import SidebarInset from
`@repo/ui/components/ui/sidebar`, replace the <div className="flex-1"> that
currently contains <Header />, the <div id="loader" ...> (with isLoading,
GlobalSpinner, Outlet), <ScrollToTopButton /> and <Footer /> with <SidebarInset
className="flex-1"> ... </SidebarInset>, preserving the id="loader" inner div
and existing children (Header, GlobalSpinner/Outlet, ScrollToTopButton, Footer)
and any className logic (cn(isLoading && "opacity-70")).
In `@apps/dashboard/src/components/layout/navigation-reset-wrapper.tsx`:
- Around line 4-12: Rename the generic exported component Wrapper to a
descriptive name (e.g., ScrollResetWrapper or NavigationScrollReset) to reflect
its behavior; update the component declaration/export (Wrapper) to the new name
and change all import sites (for example where App.tsx imports Wrapper) to the
new identifier so references remain consistent; keep the existing implementation
(useLocation, useLayoutEffect, window.scrollTo) unchanged—only change the
component name and its imports/exports to improve clarity.
In `@apps/dashboard/src/features/auth/components/layout.tsx`:
- Around line 3-8: Rename the exported React component from Layout to AuthLayout
(change the symbol exported by the component currently declared as Layout to
AuthLayout) and update all imports/usages that currently import it (notably the
place where it was imported as Layout as AuthLayout) to import AuthLayout
directly; specifically, update the component declaration name (Layout ->
AuthLayout), its export, and any references/imports in main.tsx and other
consumers so there is no aliased import.
In
`@apps/dashboard/src/features/files-circle-packing-chart/components/circular-packing.tsx`:
- Around line 34-45: The resetSVGDimensions function is recreated each render
and causes a stale listener and memo invalidation for OptionsSection; wrap
resetSVGDimensions in useCallback (e.g., const resetSVGDimensions =
useCallback(() => { ... }, [parentDivRef.current, setWidth, setHeight])) and
then keep the useEffect that adds/removes the resize listener but include
resetSVGDimensions in its dependency array so the listener always references the
memoized callback; ensure the callback reads parentDivRef.current safely and
uses setWidth/setHeight from props/state in its deps.
In
`@apps/dashboard/src/features/files-circle-packing-chart/hooks/use-animate-chart.tsx`:
- Around line 42-44: Remove the unnecessary useMemo wrappers around the trivial
multiplications: replace the computed values derived in boundaryHeightMargin and
boundaryWidthMargin (currently using useMemo with height * 1e-9 and width *
1e-9) with plain const assignments, and then remove useMemo from the import list
if it has no other usages in the file; update references to boundaryHeightMargin
and boundaryWidthMargin as needed.
In `@apps/dashboard/src/loaders/auth-loader.ts`:
- Around line 102-109: Replace the manual concatenation of Zod issue messages in
googleAuthLoader with the centralized formatter: when
validatedCallBackUrl.success is false, throw new
Error(formatZodError(validatedCallBackUrl.error)) instead of using
issues.reduce(...); ensure the function uses the existing formatZodError helper
(import or reference the same symbol used by
redirectToVSCodeAfterGoogleAuthLoader) so both loaders share consistent Zod
error formatting (note the thrown error is caught by the surrounding try/catch
in googleAuthLoader).
In `@packages/ui/src/components/ui/sidebar.tsx`:
- Around line 80-85: The cookie assignment for SIDEBAR_COOKIE_NAME in the effect
using setOpenProp/open should explicitly include SameSite and Secure attributes:
append "; SameSite=Lax" (or your chosen policy) to the cookie string and add ";
Secure" when the app is served over HTTPS (detect via location.protocol ===
'https:' or window.isSecureContext) so the code that sets document.cookie with
SIDEBAR_COOKIE_NAME and SIDEBAR_COOKIE_MAX_AGE includes these flags.
apps/dashboard/src/features/period-languages-chart/components/period-languages-chart.tsx
Show resolved
Hide resolved
apps/dashboard/src/features/period-projects/components/period-projects.tsx
Show resolved
Hide resolved
- wrapped `langagesToFetch` in a `useMemo` in the `files-list.tsx` component
- fixed the issue of the scroll to top button being moved when the dropdowns/modals are toggled by moving in the root `App.tsx` file instead of it being a part of the layout (affected by the sidebar)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/src/components/layout/layout.tsx (1)
26-29: Loading state renders spinner andOutletsimultaneously, causing large page height.When
isLoadingis true,GlobalSpinner(h-dvh— full viewport height) and<Outlet />both render inside the same div. This stacks them vertically, producing a very tall scrollable area during route transitions. Consider conditionally rendering only the spinner while loading, or positioning the spinner as an overlay.♻️ Option A: render only the spinner while loading
- <div id="loader" className={cn(isLoading && "opacity-70")}> - {isLoading && <GlobalSpinner />} - <Outlet /> - </div> + <div id="loader" className={cn(isLoading && "opacity-70")}> + {isLoading ? <GlobalSpinner /> : <Outlet />} + </div>♻️ Option B: overlay the spinner on top of existing content
- <div id="loader" className={cn(isLoading && "opacity-70")}> - {isLoading && <GlobalSpinner />} - <Outlet /> - </div> + <div id="loader" className={cn("relative", isLoading && "opacity-70")}> + {isLoading && ( + <div className="absolute inset-0 z-10 flex items-center justify-center"> + <ClipLoader size={80} color="var(--primary)" /> + </div> + )} + <Outlet /> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/layout/layout.tsx` around lines 26 - 29, The loader div currently renders both GlobalSpinner and Outlet when isLoading is true, causing the page to grow tall; update the layout so that when isLoading is true you render only the spinner (or render the spinner as an absolutely positioned overlay) — e.g., in the component that uses isLoading, GlobalSpinner and Outlet, change the JSX around the div with id="loader" (and the cn(...) usage) so that it conditionally returns <GlobalSpinner /> alone while isLoading, otherwise returns the normal <Outlet />; alternatively implement overlay behavior by wrapping Outlet in a relatively positioned container and rendering GlobalSpinner with absolute positioning (h-dvh, inset-0, flex center) so the Outlet remains underneath without increasing document height. Ensure references to isLoading, GlobalSpinner, Outlet and the div id="loader" are updated accordingly.
🤖 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/ui/src/components/scroll-to-top-button.tsx`:
- Around line 29-39: The wrapper div in ScrollToTopButton uses
"pointer-events-none", which prevents the nested Button's onClick (jumpToTop)
from firing; fix by re-enabling pointer events on the interactive element — add
"pointer-events-auto" to the Button's className (or remove the wrapper's
pointer-events-none if that wrapper behavior is unnecessary) so the Button can
receive clicks and jumpToTop will be invoked.
---
Nitpick comments:
In `@apps/dashboard/src/components/layout/layout.tsx`:
- Around line 26-29: The loader div currently renders both GlobalSpinner and
Outlet when isLoading is true, causing the page to grow tall; update the layout
so that when isLoading is true you render only the spinner (or render the
spinner as an absolutely positioned overlay) — e.g., in the component that uses
isLoading, GlobalSpinner and Outlet, change the JSX around the div with
id="loader" (and the cn(...) usage) so that it conditionally returns
<GlobalSpinner /> alone while isLoading, otherwise returns the normal <Outlet
/>; alternatively implement overlay behavior by wrapping Outlet in a relatively
positioned container and rendering GlobalSpinner with absolute positioning
(h-dvh, inset-0, flex center) so the Outlet remains underneath without
increasing document height. Ensure references to isLoading, GlobalSpinner,
Outlet and the div id="loader" are updated accordingly.
- re-enabled the click on the scroll-to-top-button by adding pointer-events-auto to the button
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ui/src/components/scroll-to-top-button.tsx`:
- Around line 29-39: The wrapper div in the ScrollToTopButton component had
pointer-events disabled; ensure the Button inside (component Button in
scroll-to-top-button.tsx) has pointer-events restored so clicks reach the
handler jumpToTop — keep the outer div className "pointer-events-none ..." and
ensure the Button has "pointer-events-auto" (and retains aria-label/title and
onClick={jumpToTop}) so the click/tap interactivity is restored while
transparent areas remain non-interactive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commits
feat: sidebar
main-layoutclass from theindex.cssand updated the main layout.tsx component by removing the useEffect using that classfeat: reset scroll on page navigation
navigation-reset-wrapper.tsxcomponent that brings the user back to the top of a page when navigatinguse-page-titlehookfix: TooltipProvider
App.tsxcomponentfix: circle packing chart
fix: page reset
feat: link + query params
link-with-querythat wraps the react-router link and adds the search params to the path navigated toupdated affected components :
app-sidebar,auth-dropdown(removed theAUTH_DROPDOWN_ITEMSconstant),navbar,title,period-projectsfix: token conflict
reset-password-tokeninstead of justtoken. This conflict lead to the page reset-password page to be stuck in an infinite loop trying to navigate to the dashboard when the reset password procedure is overfix: emails year
get-onboarding-email-bodyandget-password-reset-email-body)redirectToVSCodeAfterGoogleAuthLoaderwhen the zod validation failsfeat: new langages subset
v0.0.51Summary by CodeRabbit
New Features
Improvements
Updates