[dev] [Marfuen] feat/implement-new-design-system #2000
Conversation
PR SummaryImplements the new design system across the app and adds a scoped assistant chat history API.
Written by Cursor Bugbot for commit 9969695. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| )} | ||
| <AppShellNavItem icon={<SidebarClose className="size-4" />} onClick={toggleSidebar}> | ||
| Collapse sidebar | ||
| </AppShellNavItem> |
There was a problem hiding this comment.
Sidebar collapse state doesn't persist to server cookie
High Severity
The "Collapse sidebar" button calls toggleSidebar() from useAppShell() which only toggles the design system's internal state. Unlike the old SidebarCollapseButton which called both setIsCollapsed() to update the SidebarProvider context and updateSidebarState() to persist to the server cookie, this new implementation does neither. As a result, the sidebar collapsed state won't persist across page refreshes, and components using useSidebar().isCollapsed (like SidebarLogo on line 184) will have stale state that doesn't reflect the visual state after toggling.
| hasAuditorRole, | ||
| isOnlyAuditor, | ||
| }: AppSidebarProps) { | ||
| const pathname = usePathname(); |
There was a problem hiding this comment.
Missing null check for pathname could cause runtime crash
Medium Severity
The usePathname() hook returns string | null, but pathname is used without a null check at line 138 where pathname.split('/') is called. If pathname is null, this will throw a TypeError. The code inconsistently uses optional chaining later at line 144 (pathname?.startsWith()), indicating awareness of nullability but incomplete handling. The sibling component AppShellRailNav.tsx correctly handles this with usePathname() ?? ''.
Additional Locations (1)
…ce layout components
…/trycompai/comp into feat/implement-new-design-system
| ); | ||
| })} | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
Pagination controls removed from risks table
High Severity
The refactored RisksTable removes all pagination UI controls. The old implementation used DataTable with DataTablePagination, which provided page navigation buttons, page size selector, and current page display. The new implementation renders a basic Table that maps over all mergedRisks without any pagination controls. While the pageCount prop is still received and data fetching respects pagination query params (page, perPage), users have no way to navigate between pages - they would need to manually edit URL parameters. For organizations with many risks, this makes the table unusable beyond the first page of results.
| } | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
S3 presigned URLs computed but never used
Medium Severity
The layout generates S3 presigned URLs for all organization logos via getSignedUrl calls, but these URLs are never displayed. The logoUrls object is passed to AppShellWrapper, which destructures it but never uses it. The refactored OrganizationSwitcher now displays colored dots instead of logos and doesn't receive the logoUrls prop. This results in unnecessary S3 API calls on every page load within the app, adding latency and API costs for work that produces no user-visible result.
Additional Locations (1)
| published: 'oklch(0.6 0.16 145)', // --success (green) | ||
| draft: 'oklch(0.75 0.15 85)', // --warning (yellow) | ||
| archived: 'oklch(0.556 0 0)', // --muted-foreground (gray) | ||
| needs_review: 'oklch(0.58 0.22 27)', // --destructive (red) |
There was a problem hiding this comment.
Hardcoded chart colors break dark mode support
Medium Severity
The chart colors were changed from CSS variables (e.g., hsl(var(--chart-positive))) to hardcoded oklch values. These static color values won't respond to theme changes, meaning the charts will display with the same colors in both light and dark modes. Other charts in the codebase (like ComplianceProgressChart, PeopleChart, TasksChart) still use CSS variables for theme-adaptive colors, creating an inconsistent user experience.
Additional Locations (1)
…ctor policy components
| updatedAt: true, | ||
| }, | ||
| orderBy: [{ lastPublishedAt: 'desc' }, { updatedAt: 'desc' }], | ||
| orderBy: { name: 'desc' }, |
There was a problem hiding this comment.
Trust portal policies sorted in reverse alphabetical order
High Severity
The orderBy clause was changed from date-based sorting ([{ lastPublishedAt: 'desc' }, { updatedAt: 'desc' }]) to { name: 'desc' }. This causes policies in the trust portal to be sorted in reverse alphabetical order (Z to A) instead of by most recently published. For external trust portal users reviewing an organization's compliance policies, this is counterintuitive and likely unintended. If alphabetical sorting was desired, it would typically be 'asc' (A to Z).
…nce policy overview components
apps/app/src/app/(app)/[orgId]/policies/all/components/PolicyPageActions.tsx
Show resolved
Hide resolved
| }); | ||
| setIsStatusChangeDialogOpen(true); | ||
| setIsSubmitting(false); | ||
| }; |
There was a problem hiding this comment.
Published policy changes no longer require approval workflow
Medium Severity
The approval workflow for changes to already-published policies has been removed. Previously, modifying a published policy would trigger the approval dialog requiring an approver to be selected. Now, changes to published policies only show a simple confirmation dialog and bypass the approval process entirely. This is a significant behavioral change that may allow unauthorized modifications to published policies without proper review.
…ove PolicyFilters layout
| </span>{' '} | ||
| <span className="text-foreground">→ {current?.toString() || '(empty)'}</span> | ||
| </li> | ||
| → {String(current) || '(empty)'} |
There was a problem hiding this comment.
Audit log shows literal "null" instead of "(empty)"
Low Severity
The code changed from previous?.toString() || '(empty)' to String(previous) || '(empty)'. When previous or current is null/undefined, String(null) returns the literal string "null" which is truthy, so the || '(empty)' fallback never triggers. Users will see "null" or "undefined" as text in audit logs instead of the intended "(empty)" placeholder.
| </Suspense> | ||
| <PolicyTailoringProvider statuses={{}}> | ||
| <PolicyFilters policies={nonArchivedPolicies} /> | ||
| </PolicyTailoringProvider> |
There was a problem hiding this comment.
Policy tailoring status indicators completely broken
High Severity
The PolicyTailoringProvider is passed hardcoded empty statuses={{}}, which breaks the real-time policy tailoring status feature. The old implementation fetched onboarding data and used useRealtimeRun to subscribe to trigger.dev runs, populating the provider with live status updates. This allowed showing "Tailoring", "Queued", "Processing" indicators on policies and a progress bar during onboarding. The new page completely omits the onboarding query and real-time subscription, so tailoring status will never display and policies won't be blocked during active tailoring.
| return null; | ||
| } | ||
|
|
||
| const approverName = `${policy?.approver?.user?.name} (${policy?.approver?.user?.email})`; |
There was a problem hiding this comment.
Approver name displays "undefined" when approver is null
Low Severity
The approverName template literal doesn't handle the case where policy.approver is null. Since the type explicitly allows approver to be null, when isPendingApproval is true but approver data isn't loaded or doesn't exist, users will see "Waiting for undefined (undefined) to review and approve this policy." instead of a meaningful message.
| }, | ||
| { | ||
| id: 'people', | ||
| path: `/${organization.id}/people/all`, |
There was a problem hiding this comment.
Navigation links to deprecated route causing unnecessary redirect
Low Severity
The new navigation components link to /people/all, but this PR also adds a redirect from /people/all to /people. This creates unnecessary server-side redirects on every People navigation click. The links in AppSidebar, AppShellWrapper, and AppShellRailNav should point directly to /people instead of the deprecated /people/all route.
This is an automated pull request to merge feat/implement-new-design-system into dev.
It was created by the [Auto Pull Request] action.