refactor: Restructure policies module and enhance editor functionality - #78
Conversation
- Refactored policies pages and components with improved structure - Added new PolicyEditor with enhanced Tiptap extensions and features - Implemented new types and actions for policy management - Updated localization and UI components for policies - Added table and filter improvements for policies list - Integrated new status and empty state components
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request implements extensive modifications across the application. It adds new Tiptap table dependencies to support enhanced rich text editing and refines type imports and formatting consistency. The policy management domain received significant updates: several legacy components, pages, and hooks were removed, while new functions for retrieving, updating, and displaying policy details were introduced. The changes include updates to server actions, pagination and filtering in policy queries, and a revamped policy editor. In addition, UI elements such as tables, skeleton loaders, and localization files have been updated, and analytics initialization has been removed from provider files. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as PolicyDetailsPage
participant A as Auth Service
participant G as getPolicyDetails
participant E as PolicyDetails Component
participant UPE as usePolicyDetails Hook
participant UP as updatePolicy Function
U->>P: Request policy details page
P->>A: Authenticate user session
A-->>P: Return session info (organizationId)
P->>G: Call getPolicyDetails(policyId)
G-->>P: Return policy details or error
P->>E: Render PolicyDetails component with data
E->>UPE: Initialize editor via usePolicyDetails
U->>E: Update policy content using PolicyEditor
E->>UP: Call updatePolicy with new content
UP-->>E: Return update response
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- Wrap PostHog provider with Suspense for improved rendering - Remove unnecessary comment in index file - Import Suspense from React
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (27)
apps/app/src/utils/format.ts (1)
83-89: Consider maintaining consistent date formats.The function now uses different formats for dates in the current year ("MMM dd, yyyy") versus other years (defaulting to "P"). This inconsistency might confuse users and could affect UI layouts due to varying string lengths.
Consider using the same format for all dates:
export function formatDate(date: string, dateFormat?: string) { - if (isSameYear(new Date(), new Date(date))) { - return format(new Date(date), "MMM dd, yyyy"); - } - - return format(new Date(date), dateFormat ?? "P"); + return format(new Date(date), dateFormat ?? "MMM dd, yyyy"); }Alternatively, if different formats are intentional, consider making both formats explicit:
export function formatDate(date: string, dateFormat?: string) { if (isSameYear(new Date(), new Date(date))) { return format(new Date(date), "MMM dd, yyyy"); } - return format(new Date(date), dateFormat ?? "P"); + return format(new Date(date), dateFormat ?? "MMM dd, yyyy"); }apps/app/src/components/tables/policies/empty-states.tsx (3)
7-26: Enhance accessibility and UI/UX.Consider the following improvements:
- Add aria-label to the icon for screen readers
- Add aria-live region for dynamic content updates
- Add an icon to the "Create" button for better visual hierarchy
Apply this diff to implement the suggestions:
export function NoPolicies() { const t = useI18n(); return ( - <Card className="w-full"> + <Card className="w-full" role="region" aria-live="polite"> <CardContent className="flex flex-col items-center justify-center p-6 text-center"> - <FileText className="h-12 w-12 text-muted-foreground mb-4" /> + <FileText + className="h-12 w-12 text-muted-foreground mb-4" + aria-label={t("policies.no_policies_icon_label")} + /> <h3 className="font-semibold text-lg mb-2"> {t("policies.no_policies_title")} </h3> <p className="text-muted-foreground mb-6"> {t("policies.no_policies_description")} </p> <Link href="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/policies/new"> - <Button>{t("policies.create_first")}</Button> + <Button> + <FileText className="w-4 h-4 mr-2" /> + {t("policies.create_first")} + </Button> </Link> </CardContent> </Card> ); }
28-46: Enhance accessibility and add aria-live region.Similar to the NoPolicies component, consider adding accessibility attributes for better screen reader support.
Apply this diff to implement the suggestions:
export function NoResults({ hasFilters }: { hasFilters: boolean }) { const t = useI18n(); return ( - <Card className="w-full"> + <Card className="w-full" role="region" aria-live="polite"> <CardContent className="flex flex-col items-center justify-center p-6 text-center"> - <FileText className="h-12 w-12 text-muted-foreground mb-4" /> + <FileText + className="h-12 w-12 text-muted-foreground mb-4" + aria-label={t("common.empty_states.no_results.icon_label")} + /> <h3 className="font-semibold text-lg mb-2"> {t("common.empty_states.no_results.title")} </h3> <p className="text-muted-foreground"> {hasFilters ? t("common.empty_states.no_results.description_filters") : t("common.empty_states.no_results.description")} </p> </CardContent> </Card> ); }
7-46: Consider extracting common card styles into a shared component.Both
NoPoliciesandNoResultscomponents share similar card structure and styles. Consider creating a reusableEmptyStatecomponent to reduce code duplication.Here's how you could refactor this:
interface EmptyStateProps { icon?: React.ReactNode; title: string; description: string; action?: React.ReactNode; } function EmptyState({ icon, title, description, action }: EmptyStateProps) { return ( <Card className="w-full" role="region" aria-live="polite"> <CardContent className="flex flex-col items-center justify-center p-6 text-center"> {icon} <h3 className="font-semibold text-lg mb-2">{title}</h3> <p className="text-muted-foreground mb-6">{description}</p> {action} </CardContent> </Card> ); } export function NoPolicies() { const t = useI18n(); return ( <EmptyState icon={<FileText className="h-12 w-12 text-muted-foreground mb-4" aria-label={t("policies.no_policies_icon_label")} />} title={t("policies.no_policies_title")} description={t("policies.no_policies_description")} action={ <Link href="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/policies/new"> <Button> <FileText className="w-4 h-4 mr-2" /> {t("policies.create_first")} </Button> </Link> } /> ); } export function NoResults({ hasFilters }: { hasFilters: boolean }) { const t = useI18n(); return ( <EmptyState icon={<FileText className="h-12 w-12 text-muted-foreground mb-4" aria-label={t("common.empty_states.no_results.icon_label")} />} title={t("common.empty_states.no_results.title")} description={ hasFilters ? t("common.empty_states.no_results.description_filters") : t("common.empty_states.no_results.description") } /> ); }apps/app/src/app/[locale]/(app)/(dashboard)/policies/types/search-params.ts (1)
7-11: Consider starting page index at 1.The default page value of 0 might be confusing for users, as pagination typically starts at 1. This could affect the user experience and API consistency.
export const searchParamsCache = createSearchParamsCache({ q: parseAsString, - page: parseAsInteger.withDefault(0), + page: parseAsInteger.withDefault(1), status: parseAsString, });apps/app/src/components/tables/policies/loading.tsx (1)
28-42: LGTM! Well-implemented responsive loading states.The responsive design using
hidden md:table-celland varied skeleton widths creates a polished loading experience.Consider extracting repeated skeleton patterns.
The skeleton cell implementation could be made more maintainable by extracting it into a reusable component.
+interface SkeletonCellProps { + width: string; + skeletonWidth: string; + className?: string; +} + +function SkeletonCell({ width, skeletonWidth, className }: SkeletonCellProps) { + return ( + <TableCell className={cn(`w-[${width}] hidden md:table-cell`, className)}> + <Skeleton + className={cn(`h-3.5 w-[${skeletonWidth}]`, isEmpty && "animate-none")} + /> + </TableCell> + ); +} <TableRow key={row.id} className="h-[45px]"> <TableCell className="w-[300px]"> <Skeleton className={cn("h-3.5 w-[80%]", isEmpty && "animate-none")} /> </TableCell> - <TableCell className="w-[120px] hidden md:table-cell"> - <Skeleton - className={cn("h-3.5 w-[70%]", isEmpty && "animate-none")} - /> - </TableCell> - <TableCell className="w-[400px] hidden md:table-cell"> - <Skeleton - className={cn("h-3.5 w-[90%]", isEmpty && "animate-none")} - /> - </TableCell> - <TableCell className="w-[150px] hidden md:table-cell"> - <Skeleton - className={cn("h-3.5 w-[60%]", isEmpty && "animate-none")} - /> - </TableCell> + <SkeletonCell width="120px" skeletonWidth="70%" /> + <SkeletonCell width="400px" skeletonWidth="90%" /> + <SkeletonCell width="150px" skeletonWidth="60%" /> </TableRow>apps/web/src/app/components/pitch/pitch-carousel.tsx (1)
46-46: Consider enhancing the loading fallback UI.The basic loading div could be improved to match the application's design system and provide a better user experience.
Consider replacing with a more sophisticated loading state:
- <React.Suspense fallback={<div>Loading...</div>}> + <React.Suspense + fallback={ + <div className="flex items-center justify-center w-full h-full"> + <div className="animate-pulse space-y-4"> + <div className="h-48 w-96 bg-muted rounded" /> + <div className="h-4 w-48 bg-muted rounded mx-auto" /> + </div> + </div> + } + >apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesListSkeleton.tsx (1)
43-43: Consider making the skeleton row count configurable.The hardcoded value of 5 rows might not match the actual data size. Consider making it a prop with a default value.
- {Array.from({ length: 5 }).map((_, index) => ( + {Array.from({ length: rowCount ?? 5 }).map((_, index) => (apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/actions/get-policy-details.ts (1)
34-54: Consider optimizing the database query.The query fetches all fields from the policy relation. Consider selecting only the required fields to optimize performance.
const policy = await db.organizationPolicy.findUnique({ where: { id: policyId, organizationId, }, select: { id: true, status: true, content: true, createdAt: true, updatedAt: true, policy: { select: { - id: true, - name: true, - description: true, - slug: true, + // Select only the fields you need + name: true, + description: true, }, }, }, });apps/app/src/components/status-policies.tsx (1)
13-18: Consider using theme-based color tokens instead of hardcoded hex values.The hardcoded hex values make it difficult to maintain consistent theming across the application and could cause issues with dark mode support.
Consider using semantic color tokens from your UI system:
const STATUS_COLORS: Record<StatusType, string> = { - draft: "#ffc107", - published: "#00DC73", - archived: "#0ea5e9", - needs_review: "#ff0000", + draft: "var(--color-warning)", + published: "var(--color-success)", + archived: "var(--color-info)", + needs_review: "var(--color-error)", } as const;apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/actions/update-policy.ts (2)
47-48: Improve type safety for updateData object.Using
Record<string, any>reduces type safety. Consider creating a specific type for the update data structure.- const updateData: Record<string, any> = {}; + const updateData: Partial<{ + content: typeof content; + status: typeof status; + }> = {};
91-94: Simplify path revalidation logic.Multiple revalidation paths with similar patterns could be simplified using a helper function.
+ const paths = [ + `/policies/${policyId}`, + "/policies", + `/[locale]/policies/${policyId}`, + "/[locale]/policies" + ]; + paths.forEach(path => revalidatePath(path)); - revalidatePath(`/policies/${policyId}`); - revalidatePath("/policies"); - revalidatePath(`/[locale]/policies/${policyId}`); - revalidatePath("/[locale]/policies");apps/app/src/components/tables/policies/data-table.tsx (2)
67-70: Improve column visibility logic maintainability.The column visibility conditions are hardcoded and could become difficult to maintain as more columns are added.
Consider using a configuration object:
+const HIDDEN_COLUMNS = { + description: true, + updatedAt: true, + status: true, +}; + className={cn( - (cell.column.id === "description" || - cell.column.id === "updatedAt" || - cell.column.id === "status") && - "hidden md:table-cell", + HIDDEN_COLUMNS[cell.column.id as keyof typeof HIDDEN_COLUMNS] && + "hidden md:table-cell", )}
30-35: Improve generic type usage.The generic type parameter
TValueis unused, andTDatais overridden withPolicyType.-export function DataTable<TData, TValue>({ +export function DataTable({ columnHeaders, data, pageCount, currentPage, -}: DataTableProps<TData, TValue>) { +}: DataTableProps<PolicyType>) {Update the interface accordingly:
-interface DataTableProps<TData, TValue> { +interface DataTableProps<TData> {packages/ui/src/editor.css (1)
24-29: Eliminate duplicate.hljs-namereferences.
.hljs-nameis declared twice (on lines 24 and 27), which can complicate maintenance. Consider merging them into a single declaration block to keep the code clean.packages/db/prisma/seed.js (1)
297-298: Use logs judiciously when seeding evidence.Logging each evidence requirement might be verbose for production. Confirm that these logs are essential or limit them to dev-only.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/types/index.ts (2)
16-21: Consider restricting negative page values.
policiesInputSchemadefaultspageto 1 andper_pageto 10, but does not prevent negative inputs. Enforce a lower bound to avoid unexpected pagination issues.- page: z.number().default(1), - per_page: z.number().default(10), + page: z.number().min(1).default(1), + per_page: z.number().min(1).default(10),
31-34: Potential expansions forAppError.The narrow set of codes is sufficient for now. Consider adding more error codes as your application grows.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/components/PolicyDetails.tsx (2)
41-46: Consider using more specific types for state variables.The state variables could benefit from more specific typing:
saveStatusalready uses a union typecurrentContentis typed asany- const [currentContent, setCurrentContent] = useState<any>(null); + const [currentContent, setCurrentContent] = useState<JSONContent | null>(null);
118-138: Consider adding error boundary for editor content initialization.The try-catch block handles errors silently. Consider wrapping the editor in an error boundary component.
apps/app/src/components/editor/policy-editor.tsx (1)
27-31: Consider adding loading state management.The component manages saving and dirty states but lacks a loading state for initial content loading.
const [editorContent, setEditorContent] = useState<JSONContent[]>(content); const [isSaving, setIsSaving] = useState<boolean>(false); const [isDirty, setIsDirty] = useState<boolean>(false); + const [isLoading, setIsLoading] = useState<boolean>(true);apps/app/src/components/tables/policies/filter-toolbar.tsx (1)
143-153: Consider adding loading state to clear filters button.The clear filters button shows isPending state but could benefit from a loading indicator.
<Button variant="ghost" size="sm" onClick={handleReset} disabled={isPending} > - <X className="h-4 w-4 mr-2" /> + {isPending ? ( + <Loader2 className="h-4 w-4 mr-2 animate-spin" /> + ) : ( + <X className="h-4 w-4 mr-2" /> + )} {t("common.actions.clear")} </Button>apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicies.ts (1)
11-32: Consider unifying error checks.
Currently, the code checks!resultand then separately checksresult.serverError. You could unify them or share logic in one block if desired. This is more of a stylistic preference and has no impact on correctness.Possible unified structure:
async function fetchPolicies(input: PoliciesInput): Promise<PoliciesResponse> { const result = await getPolicies(input); + if (!result || result.serverError) { + const error: AppError = { + code: "UNEXPECTED_ERROR", + message: result?.serverError || "An unexpected error occurred", + }; + throw error; + } return result.data?.data as PoliciesResponse; }apps/app/src/components/tables/policies/columns.tsx (1)
9-20: New PolicyType definition is clear.
The nestedpolicyobject withinPolicyTypeis easy to read, though it duplicates anidat both the top level and withinpolicy. Consider consolidating if that duplication is unnecessary.apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicy.ts (3)
17-41: fetchPolicyDetails error-handling is consistent with the rest of the app.
Similar to the logic in usePolicies—consider consolidating or unifying error checks if desired.
56-80: Use consistent user-facing error handling.
Theconsole.errorapproach is fine for debugging, but consider a user notification or logging system for production to ensure visibility of errors.
91-126: Recursive content processing is well-structured.
The function handles arrays and nested attributes thoroughly. Be mindful that, if content is extremely large or deeply nested, performance might degrade.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/app/languine.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
apps/app/package.json(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/hooks/useOrganizationControl.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/components/PoliciesOverview.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/loading.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/actions/publish-policy.ts(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/layout.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/page.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/actions/get-policy-details.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/actions/update-policy.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/components/PolicyDetails.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/types/index.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policies.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policy.ts(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/actions/get-policies.ts(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/components/PoliciesTable.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/hooks/usePolicies.ts(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/layout.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/page.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesListSkeleton.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicies.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicy.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/page.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/types/index.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/types/search-params.ts(1 hunks)apps/app/src/components/editor/actions/ai.ts(0 hunks)apps/app/src/components/editor/editor.tsx(0 hunks)apps/app/src/components/editor/policy-editor.tsx(1 hunks)apps/app/src/components/status-policies.tsx(1 hunks)apps/app/src/components/tables/policies/columns.tsx(1 hunks)apps/app/src/components/tables/policies/data-table-header.tsx(1 hunks)apps/app/src/components/tables/policies/data-table.tsx(4 hunks)apps/app/src/components/tables/policies/empty-states.tsx(1 hunks)apps/app/src/components/tables/policies/filter-toolbar.tsx(4 hunks)apps/app/src/components/tables/policies/loading.tsx(1 hunks)apps/app/src/components/tables/policies/server-columns.tsx(1 hunks)apps/app/src/locales/en.ts(6 hunks)apps/app/src/locales/es.ts(6 hunks)apps/app/src/locales/fr.ts(6 hunks)apps/app/src/locales/no.ts(6 hunks)apps/app/src/locales/pt.ts(6 hunks)apps/app/src/types/actions.ts(0 hunks)apps/app/src/utils/format.ts(1 hunks)apps/portal/src/app/[locale]/providers.tsx(1 hunks)apps/web/src/app/components/pitch/pitch-carousel.tsx(1 hunks)apps/web/src/app/providers.tsx(1 hunks)packages/db/prisma/seed.js(5 hunks)packages/ui/package.json(1 hunks)packages/ui/src/editor.css(1 hunks)
💤 Files with no reviewable changes (14)
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/loading.tsx
- apps/app/src/components/editor/editor.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/components/PoliciesTable.tsx
- apps/app/src/components/editor/actions/ai.ts
- apps/app/src/types/actions.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/page.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/(overview)/components/PoliciesOverview.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policy.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/page.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/actions/get-policies.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/layout.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/all/hooks/usePolicies.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/actions/publish-policy.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/editor.css
[error] 191-191: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
🔇 Additional comments (74)
apps/app/src/components/tables/policies/empty-states.tsx (1)
1-6: LGTM! Well-organized imports.The imports are properly structured, using appropriate aliasing and specific component imports.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/layout.tsx (2)
11-19: LGTM! Clean and well-structured layout implementation.The layout implementation is clean and follows best practices:
- Proper TypeScript typing
- Efficient server-side i18n integration
- Clear component structure with SecondaryMenu and main content
14-14:Details
❓ Verification inconclusive
Verify the navigation changes and translation key.
The menu structure has been simplified to a single item. Please ensure:
- The removal of the "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/policies/all" path aligns with the new navigation structure
- The new translation key "policies.title" exists and contains the correct text
🏁 Script executed:
#!/bin/bash # Check if the new translation key exists in localization files rg -g '*.{json,ts,js}' '"policies.title"' # Check for any remaining references to the old path rg -g '*.{ts,tsx,js,jsx}' '"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/policies/all"'Length of output: 89
Action Required: Manual Verification of Navigation & Translation Keys
The updated menu structure now features a single navigational item using the translation key
"policies.title". Please manually verify that:
- The new translation key
"policies.title"exists in your localization files and contains the correct text.- The removal of the
"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/policies/all"path fully aligns with the intended navigation behavior and doesn’t negatively impact user flows.apps/portal/src/app/[locale]/providers.tsx (1)
5-5:Details
❓ Verification inconclusive
Verify the impact of removing Analytics initialization.
The removal of the
Analyticsimport and its initialization logic might affect analytics tracking functionality. Please confirm if this is intentional and if analytics tracking will still work as expected with just theAnalyticsProvider.Run the following script to check for any remaining Analytics initialization in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for Analytics initialization and usage in the codebase. # Search for Analytics initialization rg -A 5 'Analytics\.' apps/ # Search for Analytics imports rg -A 5 'import.*Analytics.*from.*@bubba/analytics' apps/Length of output: 2691
Verify Analytics Initialization Behavior
The file still imports the
AnalyticsProviderbut the explicit initialization logic has been removed. Note that tracking calls in other parts of the codebase (e.g., in waitlist actions) are still usingServerAnalytics.track. Please confirm if the removal of the explicit Analytics initialization in this client component is intentional and ensure that the analytics tracking will function correctly under the new implementation.packages/ui/package.json (1)
92-93: LGTM! Clean addition of editor-related CSS exports.The new exports for prosemirror and editor CSS are well-structured and follow the established pattern in the package.
apps/app/src/components/tables/policies/server-columns.tsx (1)
6-10: LGTM! Improved translation key organization.The changes improve semantic clarity by using more appropriate translation keys (
common.status.titleandcommon.last_updated).apps/app/package.json (1)
26-29: LGTM! Tiptap table dependencies added consistently.The new table-related dependencies are added with consistent versioning (^2.11.5), which is good for maintaining compatibility.
apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/hooks/useOrganizationControl.ts (1)
5-5: LGTM! Code style improvements.The changes improve code consistency by:
- Using explicit type import syntax
- Adding trailing commas
Also applies to: 10-11, 34-34
apps/app/src/components/tables/policies/data-table-header.tsx (2)
75-89: LGTM! Column visibility and sorting for status.The changes improve semantic clarity by using "status" instead of "published" and updating the translation key.
91-107: LGTM! Column visibility and sorting for updatedAt.The changes improve semantic clarity by using "updatedAt" instead of "ownerId" and updating the translation key.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/page.tsx (2)
18-22: Verify the redirect path change.The redirect path has been changed from "/onboarding" to "/". Please ensure this change aligns with the expected user flow when no organization ID is present.
24-26: LGTM! Server-side column headers.Good separation of concerns by fetching column headers server-side and passing them as props.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/page.tsx (2)
8-9: LGTM! Dynamic rendering and caching configuration.Appropriate configuration for dynamic content that needs to be always up-to-date.
43-47: LGTM! Cache control headers.Comprehensive cache control headers to prevent caching of sensitive policy details.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/actions/get-policy-details.ts (2)
11-20: LGTM! Action configuration with schema validation and metadata.Well-structured server action with proper schema validation and tracking metadata.
67-73: LGTM! Error handling and logging.Good practice to log errors and return a generic error message to the client.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesList.tsx (1)
68-68:Details
❓ Verification inconclusive
Verify the empty users array usage.
An empty users array is being passed to FilterToolbar. This might indicate missing functionality or an incomplete implementation.
Also applies to: 79-79
🏁 Script executed:
#!/bin/bash # Search for FilterToolbar usage and users prop implementation ast-grep --pattern 'FilterToolbar $$$users={$_}$$$'Length of output: 53
Action Required: Verify Empty Users Prop in FilterToolbar
It appears that
PoliciesList.tsxcurrently passes an empty array (users={[]}) to theFilterToolbarcomponent (at both line 68 and line 79). This might be intentional if the component is designed to handle an empty list, but it could also indicate missing functionality or incomplete data handling.
- Please double-check the
FilterToolbarcomponent:
- Verify its prop type definitions and intended behavior regarding the
usersarray.- Ensure that an empty array is acceptable or that proper fallback logic is in place.
- Confirm whether this is a temporary placeholder or if the component should be wired to actual user data.
Since our initial search for the
usersprop usage did not yield additional insights, a manual review of theFilterToolbarimplementation is advised.packages/ui/src/editor.css (1)
1-221: Overall, good use of Tailwind@applyand syntax highlighting styles.The file demonstrates well-structured, comprehensive editor styling for code blocks, headings, and layout. No major issues beyond the minor duplicates and parsing error.
🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
packages/db/prisma/seed.js (5)
19-20: Confirm data deletion of organization requirements and evidence.Deleting all
organizationControlRequirementandorganizationEvidencerecords is drastic and may not be intended for all environments. Ensure you only run this in the correct context (e.g., development).
28-28: Redundant data deletion ofevidence.You’re already deleting
organizationEvidenceabove. Confirm that this additionalawait prisma.evidence.deleteMany();line is intended, as it removes data from a different table.
87-88: Ensurefrequencyalignment with design requirements.Explicitly storing
frequencyfor policies is beneficial. Verify that the data model (and default values) match your domain’s needs (e.g., number, string).Also applies to: 96-97
222-223: Check consistency when upsertingfrequencyin control requirements.The
frequencyfield is upserted in bothcreateandupdateblocks. Make sure the domain logic or data type is consistent with other references.Also applies to: 230-231
301-306: Upsertingfrequencyfor evidence.The upsert logic consistently sets
frequencytoevidenceReq.frequency ?? null. This aligns well with other uses offrequency.Also applies to: 309-313
apps/app/src/app/[locale]/(app)/(dashboard)/policies/types/index.ts (4)
3-14: Validate date fields inpolicySchema.
createdAtandupdatedAtusez.date(). Ensure your data loading pipeline properly converts date strings. Otherwise, parse errors might occur.
23-25: TypesPolicyandPoliciesInputlook correct.The
z.inferusage ensures strong type checks for consumers of these schemas.
26-29:PoliciesResponseinterface is straightforward.The interface handles multiple policies plus a total count. This is aligned with pagination best practices.
36-45: CentralizedappErrorsobject.This is a clean pattern for standardizing error handling. Great approach for clarity and reusability.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policies.ts (7)
4-5: Imports for database andappErrorsintroduced.Code now centralizes error messages and references the shared DB instance. This improves consistency.
8-8: Added input schema validation.Using
.schema(policiesInputSchema)ensures your action enforces consistent request params.
16-17: Improved destructuring for pagination and filtering.This clarifies the parameters (
search,status,page, andper_page) for the policy query.
23-23: Use standardized unauthorized error message.Returning
appErrors.UNAUTHORIZED.messagekeeps errors consistent with the rest of your app.
28-66: Comprehensive pagination and filtering logic.Queries the organization’s policies with optional search, status filtering, skip/take pagination, and concurrency via
Promise.all. This is efficient and clean.
88-88: Returning{ policies, total }indata.This output structure is well-formed and aligns with the
PoliciesResponseinterface.
94-94: Unified error handling.Falling back to
appErrors.UNEXPECTED_ERROR.messageensures consistent messaging for unhandled errors.apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/components/PolicyDetails.tsx (1)
71-116: LGTM! Well-configured editor with essential extensions.The editor configuration is comprehensive, including necessary extensions for rich text editing and tables.
apps/app/src/components/tables/policies/filter-toolbar.tsx (1)
87-108: LGTM! Well-implemented responsive design.The component handles mobile and desktop layouts appropriately with conditional rendering.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/[policyId]/types/index.ts (1)
32-50: LGTM! Well-structured error handling.The error types and constants are well-defined with clear messages.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicies.ts (6)
3-4: No immediate issues with the new imports.
All imports look correct.
9-10: Types import structure looks good.
No concerns here; these types are clearly named and presumably well-defined in the referenced file.
36-41: Validate numeric query parameters.
The default values1and10forpageandper_pagecan cause unexpected behavior if the query strings are non-numeric or negative. Consider adding a quick numeric check or relying on server-side schema validation.
42-50: SWR usage is consistent with the new fetch function.
No issues spotted; the key structure ensures correct caching and invalidation.
57-61: Returned shape aligns with PoliciesResponse.
Exposingpolicies,total, and therevalidatePoliciesfunction is clear and maintains a straightforward API.
63-63: No further concerns at the end of the hook.
This closing brace appears to reflect a minor structural change with no functional impact.apps/app/src/components/tables/policies/columns.tsx (5)
3-4: Imports for status and date formatting look correct.
UtilizingStatusPoliciesandformatDateis appropriate for clarifying UI components and display logic.
26-30: Accessing fields onrow.original.policyis logically consistent.
No issues found; the fallback topolicy.nameis properly handled.
35-36: Link and mobile display logic are appropriate.
Usage of<Link>for navigation and<StatusPolicies>for mobile layouts ensures responsiveness.Also applies to: 40-40
47-48: Status column is straightforward.
No concerns; mapping the status toStatusPoliciesis a clean approach for consistent rendering.Also applies to: 50-50, 54-54
60-61: Date column is well-handled withformatDate.
This is a neat approach, preserving localized date formats.Also applies to: 66-67
apps/app/src/app/[locale]/(app)/(dashboard)/policies/hooks/usePolicy.ts (4)
4-6: Imports for policy details and updates appear correct.
No issues with referencing these action files and their types.
8-15: ContentNode interface is clearly structured.
This interface captures a wide variety of node attributes. No issues spotted.
44-54: SWR setup for policy details looks correct.
Using["policy-details", policyId]as the key helps differentiate from the overall policies list.
83-83: Exported return structure is intuitive.
No issues with the naming (policy,isLoading,updatePolicy,refreshPolicy); it's straightforward and descriptive.Also applies to: 86-87
apps/app/src/locales/es.ts (5)
53-54: LGTM! Status translations are clear and consistent.The translations for "inactive" and "title" are accurate and align well with Spanish language conventions.
76-76: LGTM! Empty state message improved.The change from "Sin resultados" to "No se encontraron resultados" provides better clarity and natural phrasing in Spanish.
147-153: LGTM! Common translations are accurate and consistent.The new translations for edit, errors, description, and last_updated are well-structured and natural in Spanish.
257-258: LGTM! Policy-related translations are comprehensive.The translations for policy statuses, actions, and messages are accurate and maintain consistency with the application's terminology.
Also applies to: 274-291
708-709: LGTM! Sub-page translations are clear.The translations for policy editor and details pages are accurate and consistent with the application's navigation structure.
apps/app/src/locales/pt.ts (5)
53-54: LGTM! Status translations are accurate.The translations for "inactive" and "title" are appropriate and follow Portuguese language conventions.
76-76: LGTM! Empty state message improved.The change to "Nenhum resultado encontrado" provides better clarity in Portuguese.
147-153: LGTM! Common translations are well-structured.The new translations for edit, errors, description, and last_updated are natural and accurate in Portuguese.
257-258: LGTM! Policy-related translations are comprehensive.The translations for policy management features are accurate and maintain consistency with the application's terminology in Portuguese.
Also applies to: 274-291
708-709: LGTM! Sub-page translations are clear.The translations for policy editor and details pages follow Portuguese language conventions.
apps/app/src/locales/no.ts (5)
53-54: LGTM! Status translations are accurate.The translations for "inactive" and "title" are appropriate and follow Norwegian language conventions.
76-76: LGTM! Empty state message improved.The change to "Ingen resultater funnet" provides better clarity in Norwegian.
147-153: LGTM! Common translations are well-structured.The new translations for edit, errors, description, and last_updated are natural and accurate in Norwegian.
257-258: LGTM! Policy-related translations are comprehensive.The translations for policy management features are accurate and maintain consistency with the application's terminology in Norwegian.
Also applies to: 274-291
708-709: LGTM! Sub-page translations are clear.The translations for policy editor and details pages follow Norwegian language conventions.
apps/app/src/locales/fr.ts (5)
53-54: LGTM! Status translations are accurate.The translations for "inactive" and "title" are appropriate and follow French language conventions.
76-76: LGTM! Empty state message improved.The change to "Aucun résultat trouvé" provides better clarity in French.
147-153: LGTM! Common translations are well-structured.The new translations for edit, errors, description, and last_updated are natural and accurate in French.
257-258: LGTM! Policy-related translations are comprehensive.The translations for policy management features are accurate and maintain consistency with the application's terminology in French.
Also applies to: 274-291
708-709: LGTM! Sub-page translations are clear.The translations for policy editor and details pages follow French language conventions.
apps/app/src/locales/en.ts (2)
150-155: LGTM! Well-structured common translations.Good addition of commonly used UI strings in the
commonsection. This promotes reusability and consistency across the application.
308-321: LGTM! Comprehensive policy management translations.Well-structured set of translations covering all necessary UI elements for policy management, including:
- Creation flows
- Search functionality
- Status filtering
- Empty states
- Success/error messages
|
|
||
| import { env } from "@/env.mjs"; | ||
| import { Analytics, AnalyticsProvider } from "@bubba/analytics"; | ||
| import { AnalyticsProvider } from "@bubba/analytics"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the architectural decision to remove Analytics initialization.
The removal of Analytics initialization is consistent across both web and portal apps, suggesting a deliberate architectural change. Please ensure this change is documented and that the new approach to analytics tracking is clearly communicated.
Run the following script to check for documentation about this architectural change:
🏁 Script executed:
#!/bin/bash
# Description: Check for documentation about analytics architecture.
# Search for documentation files
fd -e md -e mdx | xargs rg -i 'analytics'
# Check commit messages for context
git log -S "Analytics" --pickaxe-regex --pretty=format:'%h %s' -pLength of output: 169
Action: Document the Architectural Change for Analytics Removal
The removal of Analytics initialization in apps/web/src/app/providers.tsx is clearly intentional and consistent with the changes seen in the portal providers. However, our documentation review only surfaced a reference to an analytics image in the README, and there isn’t any explicit documentation detailing this architectural decision. Please ensure that the rationale and the new approach to analytics tracking are clearly documented (for example, by updating relevant architecture documents or commit messages) so that the intent behind these changes is transparent to all developers.
| <div className="space-y-6"> | ||
| <div className="flex items-center justify-between"> | ||
| <Skeleton className="h-8 w-32" /> | ||
| <Skeleton className="h-9 w-32" /> | ||
| </div> | ||
|
|
||
| <div className="flex gap-4 mb-6"> | ||
| <Skeleton className="h-10 flex-1 max-w-md" /> | ||
| <Skeleton className="h-10 w-[180px]" /> | ||
| </div> | ||
|
|
||
| <div className="rounded-md border"> | ||
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead> | ||
| <Skeleton className="h-5 w-32" /> | ||
| </TableHead> | ||
| <TableHead> | ||
| <Skeleton className="h-5 w-20" /> | ||
| </TableHead> | ||
| <TableHead> | ||
| <Skeleton className="h-5 w-40" /> | ||
| </TableHead> | ||
| <TableHead> | ||
| <Skeleton className="h-5 w-28" /> | ||
| </TableHead> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {Array.from({ length: 5 }).map((_, index) => ( | ||
| <TableRow key={`skeleton-row-${index + 1}`}> | ||
| <TableCell> | ||
| <Skeleton className="h-5 w-40" /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <Skeleton className="h-5 w-20" /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <Skeleton className="h-5 w-full max-w-[300px]" /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <Skeleton className="h-5 w-24" /> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> | ||
| </div> | ||
|
|
||
| <div className="flex justify-between items-center px-2 py-4"> | ||
| <Skeleton className="h-5 w-40" /> | ||
| <div className="flex gap-2"> | ||
| <Skeleton className="h-8 w-8" /> | ||
| <Skeleton className="h-8 w-8" /> | ||
| <Skeleton className="h-8 w-8" /> | ||
| <Skeleton className="h-8 w-8" /> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for better accessibility.
The skeleton loader should be properly announced to screen readers to indicate the loading state.
Add appropriate ARIA attributes:
- <div className="space-y-6">
+ <div
+ className="space-y-6"
+ role="status"
+ aria-label="Loading policies"
+ aria-busy="true"
+ >📝 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.
| <div className="space-y-6"> | |
| <div className="flex items-center justify-between"> | |
| <Skeleton className="h-8 w-32" /> | |
| <Skeleton className="h-9 w-32" /> | |
| </div> | |
| <div className="flex gap-4 mb-6"> | |
| <Skeleton className="h-10 flex-1 max-w-md" /> | |
| <Skeleton className="h-10 w-[180px]" /> | |
| </div> | |
| <div className="rounded-md border"> | |
| <Table> | |
| <TableHeader> | |
| <TableRow> | |
| <TableHead> | |
| <Skeleton className="h-5 w-32" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-20" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-40" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-28" /> | |
| </TableHead> | |
| </TableRow> | |
| </TableHeader> | |
| <TableBody> | |
| {Array.from({ length: 5 }).map((_, index) => ( | |
| <TableRow key={`skeleton-row-${index + 1}`}> | |
| <TableCell> | |
| <Skeleton className="h-5 w-40" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-20" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-full max-w-[300px]" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-24" /> | |
| </TableCell> | |
| </TableRow> | |
| ))} | |
| </TableBody> | |
| </Table> | |
| </div> | |
| <div className="flex justify-between items-center px-2 py-4"> | |
| <Skeleton className="h-5 w-40" /> | |
| <div className="flex gap-2"> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| </div> | |
| </div> | |
| </div> | |
| <div | |
| className="space-y-6" | |
| role="status" | |
| aria-label="Loading policies" | |
| aria-busy="true" | |
| > | |
| <div className="flex items-center justify-between"> | |
| <Skeleton className="h-8 w-32" /> | |
| <Skeleton className="h-9 w-32" /> | |
| </div> | |
| <div className="flex gap-4 mb-6"> | |
| <Skeleton className="h-10 flex-1 max-w-md" /> | |
| <Skeleton className="h-10 w-[180px]" /> | |
| </div> | |
| <div className="rounded-md border"> | |
| <Table> | |
| <TableHeader> | |
| <TableRow> | |
| <TableHead> | |
| <Skeleton className="h-5 w-32" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-20" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-40" /> | |
| </TableHead> | |
| <TableHead> | |
| <Skeleton className="h-5 w-28" /> | |
| </TableHead> | |
| </TableRow> | |
| </TableHeader> | |
| <TableBody> | |
| {Array.from({ length: 5 }).map((_, index) => ( | |
| <TableRow key={`skeleton-row-${index + 1}`}> | |
| <TableCell> | |
| <Skeleton className="h-5 w-40" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-20" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-full max-w-[300px]" /> | |
| </TableCell> | |
| <TableCell> | |
| <Skeleton className="h-5 w-24" /> | |
| </TableCell> | |
| </TableRow> | |
| ))} | |
| </TableBody> | |
| </Table> | |
| </div> | |
| <div className="flex justify-between items-center px-2 py-4"> | |
| <Skeleton className="h-5 w-40" /> | |
| <div className="flex gap-2"> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| <Skeleton className="h-8 w-8" /> | |
| </div> | |
| </div> | |
| </div> |
| className={cn("size-2.5")} | ||
| style={{ backgroundColor: STATUS_COLORS[status] ?? " " }} | ||
| /> |
There was a problem hiding this comment.
Fix the empty space fallback.
The fallback for missing status colors uses two spaces which is not a valid color value.
- style={{ backgroundColor: STATUS_COLORS[status] ?? " " }}
+ style={{ backgroundColor: STATUS_COLORS[status] ?? "transparent" }}📝 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.
| className={cn("size-2.5")} | |
| style={{ backgroundColor: STATUS_COLORS[status] ?? " " }} | |
| /> | |
| className={cn("size-2.5")} | |
| style={{ backgroundColor: STATUS_COLORS[status] ?? "transparent" }} | |
| /> |
| const updateData: Record<string, any> = {}; | ||
|
|
||
| if (content !== undefined) { | ||
| console.log("CONTENT TYPE:", typeof content); |
There was a problem hiding this comment.
Remove console.log statements before production deployment.
Debug statements should not be present in production code.
- console.log("CONTENT TYPE:", typeof content);
- console.log("Extracted content array from TipTap doc");
- console.log("Unknown content format - using as is");
- console.log("Updating policy with data:", JSON.stringify(updateData));Also applies to: 55-55, 59-59, 78-78
| {policies.length > 0 ? ( | ||
| <DataTable | ||
| columnHeaders={columnHeaders} | ||
| data={policies as unknown as PolicyType[]} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid type casting with unknown.
Using as unknown as PolicyType[] is a type-safety escape hatch that could hide potential issues.
Consider updating the usePolicies hook to return the correct type directly:
- data={policies as unknown as PolicyType[]}
+ data={policies}Update the usePolicies hook return type to match PolicyType[].
📝 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.
| data={policies as unknown as PolicyType[]} | |
| data={policies} |
| const debouncedSave = useDebouncedCallback(async (content: any) => { | ||
| if (!policy) return; | ||
|
|
||
| setSaveStatus("Saving"); | ||
| try { | ||
| const contentToSave = | ||
| content.type === "doc" && Array.isArray(content.content) | ||
| ? content.content | ||
| : content; | ||
|
|
||
| await updatePolicy({ | ||
| ...policy, | ||
| content: contentToSave, | ||
| }); | ||
|
|
||
| setSaveStatus("Saved"); | ||
| } catch (err) { | ||
| console.error("Failed to save policy:", err); | ||
| setSaveStatus("Unsaved"); | ||
| } | ||
| }, 1000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the save function.
The catch block only logs the error without providing user feedback.
} catch (err) {
console.error("Failed to save policy:", err);
setSaveStatus("Unsaved");
+ // Add user notification
+ toast({
+ title: "Error saving policy",
+ description: "Please try again or contact support if the issue persists.",
+ variant: "destructive",
+ });
}📝 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.
| const debouncedSave = useDebouncedCallback(async (content: any) => { | |
| if (!policy) return; | |
| setSaveStatus("Saving"); | |
| try { | |
| const contentToSave = | |
| content.type === "doc" && Array.isArray(content.content) | |
| ? content.content | |
| : content; | |
| await updatePolicy({ | |
| ...policy, | |
| content: contentToSave, | |
| }); | |
| setSaveStatus("Saved"); | |
| } catch (err) { | |
| console.error("Failed to save policy:", err); | |
| setSaveStatus("Unsaved"); | |
| } | |
| }, 1000); | |
| const debouncedSave = useDebouncedCallback(async (content: any) => { | |
| if (!policy) return; | |
| setSaveStatus("Saving"); | |
| try { | |
| const contentToSave = | |
| content.type === "doc" && Array.isArray(content.content) | |
| ? content.content | |
| : content; | |
| await updatePolicy({ | |
| ...policy, | |
| content: contentToSave, | |
| }); | |
| setSaveStatus("Saved"); | |
| } catch (err) { | |
| console.error("Failed to save policy:", err); | |
| setSaveStatus("Unsaved"); | |
| // Add user notification | |
| toast({ | |
| title: "Error saving policy", | |
| description: "Please try again or contact support if the issue persists.", | |
| variant: "destructive", | |
| }); | |
| } | |
| }, 1000); |
| onUpdate: ({ editor }) => { | ||
| try { | ||
| const json = editor.getJSON().content; | ||
| if (json) { | ||
| setEditorContent(json as JSONContent[]); | ||
| setIsDirty(true); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error updating editor content:", error); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in onUpdate callback.
The error is only logged to console without user feedback.
} catch (error) {
console.error("Error updating editor content:", error);
+ toast({
+ title: "Error updating content",
+ description: "Changes may not be saved. Please refresh the page.",
+ variant: "destructive",
+ });
}📝 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.
| onUpdate: ({ editor }) => { | |
| try { | |
| const json = editor.getJSON().content; | |
| if (json) { | |
| setEditorContent(json as JSONContent[]); | |
| setIsDirty(true); | |
| } | |
| } catch (error) { | |
| console.error("Error updating editor content:", error); | |
| } | |
| }, | |
| onUpdate: ({ editor }) => { | |
| try { | |
| const json = editor.getJSON().content; | |
| if (json) { | |
| setEditorContent(json as JSONContent[]); | |
| setIsDirty(true); | |
| } | |
| } catch (error) { | |
| console.error("Error updating editor content:", error); | |
| toast({ | |
| title: "Error updating content", | |
| description: "Changes may not be saved. Please refresh the page.", | |
| variant: "destructive", | |
| }); | |
| } | |
| }, |
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| disabled={isSaving || !isDirty} | ||
| className={cn("gap-1", isSaving && "opacity-50 cursor-not-allowed")} | ||
| > | ||
| <Save className="w-4 h-4" /> | ||
| {isSaving ? "Saving..." : "Save"} | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
Add save handler for the save button.
The save button is rendered but lacks an onClick handler.
<Button
variant="outline"
size="sm"
disabled={isSaving || !isDirty}
+ onClick={handleSave}
className={cn("gap-1", isSaving && "opacity-50 cursor-not-allowed")}
>📝 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.
| <Button | |
| variant="outline" | |
| size="sm" | |
| disabled={isSaving || !isDirty} | |
| className={cn("gap-1", isSaving && "opacity-50 cursor-not-allowed")} | |
| > | |
| <Save className="w-4 h-4" /> | |
| {isSaving ? "Saving..." : "Save"} | |
| </Button> | |
| </div> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| disabled={isSaving || !isDirty} | |
| onClick={handleSave} | |
| className={cn("gap-1", isSaving && "opacity-50 cursor-not-allowed")} | |
| > | |
| <Save className="w-4 h-4" /> | |
| {isSaving ? "Saving..." : "Save"} | |
| </Button> | |
| </div> |
| export const policyDetailsSchema = z.object({ | ||
| id: z.string(), | ||
| status: z.enum(["draft", "published", "archived"]), | ||
| content: z.array(z.any()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for policy content.
Using z.any() for content reduces type safety. Consider defining a specific schema for the content structure.
- content: z.array(z.any()),
+ content: z.array(z.object({
+ type: z.string(),
+ attrs: z.record(z.unknown()).optional(),
+ content: z.array(z.unknown()).optional(),
+ })),📝 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.
| content: z.array(z.any()), | |
| content: z.array(z.object({ | |
| type: z.string(), | |
| attrs: z.record(z.unknown()).optional(), | |
| content: z.array(z.unknown()).optional(), | |
| })), |
| statuses: { | ||
| draft: "Draft", | ||
| published: "Published", | ||
| archived: "Archived", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate status translation detected.
The "Archived" status is defined in two locations:
policies.table.statuses.archivedpolicies.status.archived
This duplication could lead to inconsistencies in translations and maintenance overhead.
Consider consolidating these into a single translation key under common.status since status strings should be reusable across different contexts:
- policies: {
- table: {
- statuses: {
- archived: "Archived",
- }
- },
- status: {
- archived: "Archived",
- }
- }
+ common: {
+ status: {
+ archived: "Archived"
+ }
+ }Also applies to: 305-305
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/app/src/components/policies/charts/policies-by-framework.tsx (1)
25-36: Consider enhancing the empty state message.While the empty state handling is correct, the message "No frameworks have linked policies" could be more descriptive to help users understand what frameworks mean in this context.
- No frameworks have linked policies + No compliance frameworks (SOC 2, ISO 27001, GDPR) have any linked policiespackages/analytics/src/components/provider.tsx (1)
37-42: Consider enhancing the Suspense implementation.While adding Suspense is good for handling async operations, the current implementation could be improved:
- Using
nullas fallback might cause layout shifts. Consider using a minimal placeholder.- The Suspense boundary around PHProvider might affect analytics collection if the provider suspends.
- Missing error handling for potential analytics initialization failures.
Consider this enhanced implementation:
+ import { ErrorBoundary } from 'react-error-boundary'; + const AnalyticsFallback = () => ( + <div style={{ width: '100%', height: '100%' }} aria-hidden="true" /> + ); + const ErrorFallback = () => { + // Silently handle analytics errors while logging them + console.error('Analytics failed to load'); + return null; + }; return ( - <Suspense fallback={null}> + <ErrorBoundary FallbackComponent={ErrorFallback}> + <Suspense fallback={<AnalyticsFallback />}> <PHProvider client={posthog}> <PostHogPageView /> {children} </PHProvider> + </Suspense> + </ErrorBoundary> - </Suspense> );apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesList.tsx (2)
34-35: Extract pagination defaults to constants.Consider extracting the default values for
per_pageandpageto named constants at the top of the file for better maintainability.+const DEFAULT_PER_PAGE = 10; +const DEFAULT_PAGE = 1; + export function PoliciesList({ columnHeaders }: PoliciesListProps) { // ... - const per_page = Number(searchParams.get("per_page")) || 10; - const page = Number(searchParams.get("page")) || 1; + const per_page = Number(searchParams.get("per_page")) || DEFAULT_PER_PAGE; + const page = Number(searchParams.get("page")) || DEFAULT_PAGE;
43-54: Extract error card to a reusable component.Consider extracting the error card to a separate reusable component since error states are commonly needed across different parts of the application.
+interface ErrorCardProps { + error: Error; +} + +function ErrorCard({ error }: ErrorCardProps) { + const t = useI18n(); + return ( + <div className="p-6"> + <Card> + <CardContent className="p-6 flex items-center gap-3"> + <AlertTriangle className="text-red-500 h-5 w-5" /> + <span>{error.message || t("common.errors.unexpected_error")}</span> + </CardContent> + </Card> + </div> + ); +} export function PoliciesList({ columnHeaders }: PoliciesListProps) { // ... if (error) { - return ( - <div className="p-6"> - <Card> - <CardContent className="p-6 flex items-center gap-3"> - <AlertTriangle className="text-red-500 h-5 w-5" /> - <span>{error.message || t("common.errors.unexpected_error")}</span> - </CardContent> - </Card> - </div> - ); + return <ErrorCard error={error} />; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesList.tsx(1 hunks)apps/app/src/components/policies/charts/policies-by-framework.tsx(4 hunks)apps/app/src/components/policies/policy-overview.tsx(0 hunks)apps/app/src/components/tables/policies/data-table.tsx(4 hunks)apps/web/src/app/pitch/page.tsx(0 hunks)packages/analytics/src/components/page-view.tsx(0 hunks)packages/analytics/src/components/provider.tsx(2 hunks)packages/analytics/src/index.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/analytics/src/components/page-view.tsx
- packages/analytics/src/index.ts
- apps/web/src/app/pitch/page.tsx
- apps/app/src/components/policies/policy-overview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/app/src/components/tables/policies/data-table.tsx
🔇 Additional comments (6)
apps/app/src/components/policies/charts/policies-by-framework.tsx (3)
16-16: LGTM! Improved variable naming.The change from
datatopoliciesprovides better semantic meaning and aligns with the domain model.
46-46: LGTM! Consistent variable naming.The change maintains consistency with the earlier variable renaming while preserving the expected data structure for the BarChart component.
76-78: LGTM! Simplified mapping logic.The change to use
policy.iddirectly improves code clarity by eliminating the unnecessaryentryabstraction.packages/analytics/src/components/provider.tsx (1)
5-5: LGTM! Clean import addition.The Suspense import is correctly grouped with existing React imports, following best practices.
apps/app/src/app/[locale]/(app)/(dashboard)/policies/components/PoliciesList.tsx (2)
84-84: Avoid type casting with unknown.Using
as unknown as PolicyType[]is a type-safety escape hatch that could hide potential issues.
85-85: Use Math.ceil for pageCount calculation.The pageCount calculation should use
Math.ceilto handle cases where total is not perfectly divisible by per_page.
| <NoPolicies /> | ||
| <Loading isEmpty /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant Loading component.
The Loading component with isEmpty prop appears redundant when already showing NoPolicies component. Consider removing it to avoid potential confusion.
<FilterToolbar isEmpty={true} users={[]} />
<NoPolicies />
- <Loading isEmpty />📝 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.
| <NoPolicies /> | |
| <Loading isEmpty /> | |
| <FilterToolbar isEmpty={true} users={[]} /> | |
| <NoPolicies /> |
This branch was successfully deployed
2 participants
Summary by CodeRabbit
New Features
PoliciesListandPolicyDetails.Style and Localization
Refactor