update layout of evidence#90
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several component restructurings and new features across the evidence module. The EvidenceDetails component has been simplified to use an alert-based layout that omits publish functionality. New components such as EditEvidencePage, EditEvidenceForm, and layout wrappers for both evidence details and overview have been added. A series of visualization components now incorporate internationalization support and helper functions for dynamic color and label management. Additionally, the pull request includes updates to middleware behavior, localization keys, linting scripts, and various hooks with adjusted import paths. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant EP as EditEvidencePage
participant EF as EditEvidenceForm
participant H as useOrganizationEvidence
participant S as Server/API
U->>EP: Navigate to Edit Evidence Page
EP->>EF: Render EditEvidenceForm
EF->>H: Request evidence data (ID)
H->>S: Fetch evidence details
S-->>H: Return evidence data
H-->>EF: Provide evidence data
EF->>U: Render form with files and URL sections
sequenceDiagram
participant R as Request
participant M as Middleware
participant I as I18nMiddleware
R->>M: Incoming HTTP request
M->>M: Perform session checks and URL redirection
M->>I: Invoke I18nMiddleware(request)
I-->>M: Processed request with i18n context
M->>M: Set "x-pathname" header with request.nextUrl.pathname
M-->>R: Respond with enhanced headers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/HorizontalBarChart.tsx (1)
42-44: 🛠️ Refactor suggestionRemove debug console logs before production
There are several debug console logs that should be removed before pushing to production.
Remove the debug console logs:
- // Debug: Log the incoming data - useEffect(() => { - console.log("HorizontalBarChart - data:", data); - }, [data]); // Further down in the code: - // Debug: Log the filtered data - useEffect(() => { - console.log("HorizontalBarChart - filteredData:", filteredData); - }, [filteredData]);Also applies to: 69-72
🧹 Nitpick comments (11)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/layout.tsx (2)
13-19: Consider enhancing the evidence ID regex pattern.The current regex pattern
/\/evidence\/[a-z0-9]+/only matches lowercase letters and numbers. If your evidence IDs could contain other characters (like hyphens, underscores, or uppercase letters), this pattern might be too restrictive.- const isEvidenceDetails = /\/evidence\/[a-z0-9]+/.test(pathname); + const isEvidenceDetails = /\/evidence\/[\w-]+/.test(pathname);
15-19: Simplify path parsing logic for better readability.The current code for extracting the evidence ID is somewhat complex. Consider using a more direct approach with a regex capture group or URL parsing for better readability and maintainability.
- const pathParts = pathname.split("/"); - const evidenceIndex = pathParts.findIndex((part) => part === "evidence"); - const id = - evidenceIndex >= 0 && pathParts.length > evidenceIndex + 1 - ? pathParts[evidenceIndex + 1] - : ""; + // Extract ID directly using regex + const evidenceIdMatch = pathname.match(/\/evidence\/([^\/]+)/); + const id = evidenceIdMatch ? evidenceIdMatch[1] : "";apps/app/src/middleware.ts (1)
40-40: Consider URL encoding the pathname value.The current implementation sets the raw pathname to the header. To handle special characters properly, consider URL encoding the pathname value.
- response.headers.set("x-pathname", request.nextUrl.pathname); + response.headers.set("x-pathname", encodeURIComponent(request.nextUrl.pathname));Then decode it when used in components:
// In components that use this header const pathname = decodeURIComponent(headersList.get("x-pathname") || "");apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/HorizontalBarChart.tsx (2)
111-115: Consider abstracting bar width calculation logic to improve readabilityThe bar width calculation could be made more readable and maintainable by extracting it to a dedicated function.
Consider refactoring to use a helper function:
+ const calculateBarWidthAndPosition = (value, index, data) => { + const barWidth = (value / totalWidth) * 100; + const currentPosition = data + .slice(0, index) + .reduce((sum, item) => sum + (item.value / totalWidth) * 100 + gap, 0); + return { barWidth, position: currentPosition }; + }; {filteredData.map((d, index) => { - const barWidth = (d.value / totalWidth) * 100; - const xPosition = cumulativeWidth; - cumulativeWidth += barWidth + gap; + const { barWidth, position } = calculateBarWidthAndPosition(d.value, index, filteredData); + return ( <div key={d.key} className="relative" style={{ width: `${barWidth}%`, height: `${barHeight}px`, - left: `${xPosition}%`, + left: `${position}%`, position: "absolute", }} >
144-158: Legend implementation looks good but consider accessibility improvementsThe legend implementation is clean and correctly uses the
showLegendprop to conditionally render. However, it could benefit from improved accessibility attributes.Enhance the legend with ARIA attributes for better accessibility:
{showLegend && ( - <div className="flex flex-wrap gap-3 text-xs text-muted-foreground mt-2"> + <div className="flex flex-wrap gap-3 text-xs text-muted-foreground mt-2" role="list" aria-label="Chart legend"> {filteredData.map((item) => ( <div key={`legend-${String(item.key)}`} - className="flex items-center gap-1" + className="flex items-center gap-1" + role="listitem" > - <div className="size-2" style={{ backgroundColor: item.color }} /> + <div + className="size-2" + style={{ backgroundColor: item.color }} + aria-hidden="true" + /> <span> {item.label} ({valueFormatter(item.value)}) </span> </div> ))} </div> )}apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/EvidenceOverview.tsx (1)
15-17: Consider enhancing loading state with skeleton loadersThe current loading state is very basic. Consider using skeleton loaders to provide a more appealing loading experience.
Enhance the loading state with skeleton loaders:
if (isLoading) { - return <div>Loading...</div>; + return ( + <div className="space-y-8"> + <h2 className="text-2xl font-semibold"> + <div className="h-8 w-48 bg-gray-200 rounded animate-pulse" /> + </h2> + <div className="grid grid-cols-1 md:grid-cols-2 gap-8"> + {[1, 2, 3].map((i) => ( + <Card key={i} className={i === 3 ? "md:col-span-2" : ""}> + <CardHeader> + <div className="h-6 w-32 bg-gray-200 rounded animate-pulse" /> + </CardHeader> + <CardContent> + <div className="h-28 w-full bg-gray-200 rounded animate-pulse" /> + </CardContent> + </Card> + ))} + </div> + </div> + ); }apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx (2)
154-163: Publish button implementation is good but consider feedback enhancementThe publish button is well-implemented with proper loading state handling. However, there's no visual feedback to indicate when an evidence item is already published.
Consider adding a visual indicator for published evidence:
+ {evidence.published && ( + <div className="flex items-center gap-2 text-sm text-green-600 w-fit self-end"> + <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" className="lucide lucide-check-circle"> + <path d="M22 11.08V12a10 10 0 1 1-5.93-9.14" /> + <polyline points="22 4 12 14.01 9 11.01" /> + </svg> + Published + </div> + )} {!evidence.published && ( <Button size="sm" className="w-fit self-end" onClick={() => publishAction({ id })} disabled={isExecuting} > {isExecuting ? "Publishing..." : "Publish"} </Button> )}
64-166: Consider adding a confirmation dialog for publishingPublishing evidence might be a significant action that should require confirmation, especially if it has implications for compliance or reviews.
Consider adding a confirmation dialog before publishing:
+ import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle, DialogTrigger } from "@bubba/ui/dialog"; + import { useState } from "react"; // Within the component: + const [showPublishDialog, setShowPublishDialog] = useState(false); + + const handlePublish = () => { + publishAction({ id }); + setShowPublishDialog(false); + }; // Replace the current publish button with: {!evidence.published && ( - <Button - size="sm" - className="w-fit self-end" - onClick={() => publishAction({ id })} - disabled={isExecuting} - > - {isExecuting ? "Publishing..." : "Publish"} - </Button> + <Dialog open={showPublishDialog} onOpenChange={setShowPublishDialog}> + <DialogTrigger asChild> + <Button size="sm" className="w-fit self-end"> + Publish + </Button> + </DialogTrigger> + <DialogContent> + <DialogHeader> + <DialogTitle>Publish Evidence</DialogTitle> + <DialogDescription> + Publishing this evidence will make it visible to reviewers. Are you sure you want to proceed? + </DialogDescription> + </DialogHeader> + <DialogFooter> + <Button variant="outline" onClick={() => setShowPublishDialog(false)}> + Cancel + </Button> + <Button onClick={handlePublish} disabled={isExecuting}> + {isExecuting ? "Publishing..." : "Publish"} + </Button> + </DialogFooter> + </DialogContent> + </Dialog> )}apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/DepartmentBarChart.tsx (2)
209-239: Inconsistency between color systemsThe getStatusColor function returns hardcoded hex values which don't match the CSS variables used in STATUS_COLORS. This creates a maintenance challenge as color updates would need to be made in multiple places.
Consider refactoring to use CSS variables consistently:
function getStatusColor(status: StatusType): string { switch (status) { - case "upToDate": - return "#10b981"; // Green - case "draft": - return "#f59e0b"; // Amber - case "needsReview": - return "#ef4444"; // Red - case "empty": - return "#6b7280"; // Gray - default: - return "#6b7280"; // Default gray + case "upToDate": + return "var(--primary)"; // Green + case "draft": + return "var(--chart-open)"; // Amber + case "needsReview": + return "hsl(var(--destructive))"; // Red + case "empty": + return "var(--chart-pending)"; // Gray + default: + return "var(--chart-pending)"; // Default gray } }Alternatively, consider creating a shared helper function across all charts to avoid duplication.
210-210: Type the translation function properlyThe
tparameter in getStatusLabel is typed asany, which loses type safety.Consider importing the proper type from your i18n library:
- function getStatusLabel(status: StatusType, t: any): string { + import type { Translator } from "@/locales/client"; + function getStatusLabel(status: StatusType, t: Translator): string {apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/AssigneeBarChart.tsx (1)
225-255: Performance consideration: Console logging in productionThere are numerous console.log statements throughout the chart components which should be removed before production deployment.
Consider removing all console.log statements or wrapping them in a debug utility that only logs in development:
// Create a utility function in a shared file export function debugLog(...args: any[]): void { if (process.env.NODE_ENV === 'development') { console.log(...args); } } // Then replace console.log calls: - console.log(`Status counts for ${assigneeId}:`, statusCounts); + debugLog(`Status counts for ${assigneeId}:`, statusCounts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockbis excluded by!**/bun.lockbyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx(6 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/AssigneeBarChart.tsx(8 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/DepartmentBarChart.tsx(8 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/EvidenceOverview.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/FrameworkBarChart.tsx(7 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/HorizontalBarChart.tsx(5 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/layout.tsx(1 hunks)apps/app/src/locales/en.ts(1 hunks)apps/app/src/middleware.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx
🔇 Additional comments (35)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/list/layout.tsx (1)
17-17: Translation key updated for consistency.The translation key has been updated from "evidence.dashboard" to "evidence.dashboard.layout", which aligns with the hierarchical structure of the localization keys seen in other files.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/layout.tsx (1)
1-26: New layout component establishes consistent navigation structure.The new
Layoutcomponent properly implements the secondary menu navigation and maintains consistency with the other evidence-related pages. It correctly uses the translation keys and provides appropriate spacing for the content.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/layout.tsx (1)
23-34: Good use of conditional back button for improved navigation.The implementation of the secondary menu with a conditional back button is well done. This provides a clear navigation path for users to return to the evidence list, improving the overall user experience.
apps/app/src/middleware.ts (1)
39-41:Details
❓ Verification inconclusive
Middleware order change and custom header addition.
Moving the I18nMiddleware to run after session checks and adding the custom
x-pathnameheader is a good approach to provide path information to components. This enables the dynamic path extraction in the evidence detail layout.Verify that all components that rely on internationalization continue to work correctly with this new ordering.
🏁 Script executed:
#!/bin/bash # Check for components that might be affected by the middleware change # Look for components using internationalization that might be impacted echo "Checking components using internationalization:" rg -l "useI18n|getI18n" --type=tsx apps/app/src/app # Check if there are any other places using the x-pathname header echo -e "\nChecking for components using x-pathname header:" rg -l "x-pathname" --type=tsx apps/app/src/Length of output: 371
Below is a revised review comment. I’ve also provided an updated shell script that removes the TSX file type specification so you can re-run the checks without the “unrecognized file type” error. Please run the script below and manually verify that components using internationalization (e.g. those relying on hooks like
useI18norgetI18n) are still functioning correctly, and that any component reading the customx-pathnameheader behaves as expected.
Middleware ordering and header addition: Manual Verification Needed
Moving the
I18nMiddlewareafter session checks and adding the customx-pathnameheader appears to be a solid approach for providing path information to components such as the evidence detail layout. However, please verify that:
- All components that rely on internationalization (e.g. via
useI18norgetI18n) continue to work correctly.- The new
x-pathnameheader is correctly consumed wherever it is expected.To assist with this, please run the following shell script (which omits the problematic TSX file type flag) to locate references to these:
After executing the script, manually investigate any relevant findings to ensure that no issues arise from the middleware order change.
🏁 Script executed:
#!/bin/bash set -e echo "Checking for components using internationalization (primitive search):" rg -l "useI18n|getI18n" apps/app/src/app echo -e "\nChecking for usage of x-pathname header (primitive search):" rg -l "x-pathname" apps/app/src/Length of output: 3958
Middleware Ordering and Header Addition Verification
The updated middleware ordering—executing
I18nMiddlewareafter session checks—and the addition of the customx-pathnameheader appear correct. The static code searches confirm that:
- Numerous components (e.g., in
apps/app/src/app/[locale]/(app)/(dashboard)/...) are using internationalization hooks (useI18n/getI18n).- The
x-pathnameheader is referenced in both the middleware and in key locations such as the evidence layout (apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/layout.tsx).Please perform a runtime/manual verification that these components continue to function correctly with the updated middleware order and header addition.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/page.tsx (1)
42-64: The Card layout implementation looks goodThe Card layout structure is well-organized with proper component hierarchy using CardHeader, CardTitle, and CardContent. The spacing between sections is consistent, and the component correctly passes the necessary props to child components.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/HorizontalBarChart.tsx (2)
20-20: Props interface update for showLegend looks goodAdding the optional
showLegendprop with a default value offalseis a clean way to implement conditional rendering of the legend.Also applies to: 39-39
81-84: Bar width calculation logic change looks goodThe new approach to calculating bar widths and managing gaps between segments provides better control over the visualization.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/EvidenceOverview.tsx (3)
8-9: Good internationalization implementationThe addition of the
useI18nhook and the usage of translation keys instead of hardcoded strings is a good practice for supporting multiple languages.Also applies to: 13-13
29-31: Heading element looks good with i18nThe main heading is now properly using the internationalization function for the title.
35-63: Card-based layout is well-structuredThe refactoring to use Card components provides a more consistent and structured layout. The grid layout and spacing are well-considered, and the responsive design (md: breakpoints) is appropriate.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx (4)
4-16: Import organization is improvedThe reorganization of imports provides better readability and follows a consistent pattern of grouping related imports.
53-61: Publishing functionality looks well-implementedThe addition of the publish action with proper loading state, success, and error handling is well-implemented.
65-73: Card header structure is improvedThe updated card header with a main title and subtitle provides better information hierarchy and context for the user.
93-153: Grid layout is more structured than the previous flexbox approachChanging from flexbox to grid provides a cleaner, more predictable layout for the different sections.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (4)
3-3: LGTM: Good choice using the Alert component systemThe migration from Card components to Alert provides a more appropriate visual structure for presenting evidence details.
19-22: Improved loading state with visual hierarchyUsing different skeleton heights creates a better visual hierarchy during loading, making the loading state feel more natural and intentional.
31-33: LGTM: Simplified data refresh logicThe handleMutate function is now more focused and only defined after the loading/data checks, which is cleaner.
37-68: Improved evidence presentation with semantic markupThe new Alert-based layout provides a more focused and contextually appropriate way to display evidence information. The combination of icon, title, and status indicators creates a clearer information hierarchy.
The status indicators with appropriate icons and colors also improve the user experience by making the current state more immediately recognizable.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/DepartmentBarChart.tsx (4)
8-8: LGTM: Added internationalization supportGood addition of the useI18n hook for proper internationalization.
14-20: LGTM: Improved color system with CSS variablesUsing CSS variables for status colors improves theme consistency across components and supports better theming capabilities.
120-122: LGTM: Improved internationalization support for department namesGood implementation of both fallback to DEPARTMENT_NAMES and translations via t function.
169-185: Enhanced visualization with status legendThe addition of a legend improves clarity by providing context for the color coding in the chart.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/FrameworkBarChart.tsx (3)
7-7: LGTM: Added internationalization supportGood addition of the useI18n hook for proper internationalization.
13-19: LGTM: Improved color system with CSS variablesUsing CSS variables for status colors improves theme consistency across components and supports better theming capabilities.
153-169: Enhanced visualization with status legendThe addition of a legend improves clarity by providing context for the color coding in the chart.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/AssigneeBarChart.tsx (3)
7-7: LGTM: Added internationalization supportGood addition of the useI18n hook for proper internationalization.
14-20: LGTM: Improved color system with CSS variablesUsing CSS variables for status colors improves theme consistency across components and supports better theming capabilities.
185-201: Enhanced visualization with status legendThe addition of a legend improves clarity by providing context for the color coding in the chart.
apps/app/src/locales/en.ts (7)
791-791: Title simplification improves clarity and consistency.The title has been simplified from "Evidence Tasks" to just "Evidence", which provides a cleaner and more consistent naming convention throughout the application.
793-795: Good addition of key navigation sections.Adding dedicated localization keys for "Overview" and "Edit Evidence" provides better support for these important user journeys within the evidence module.
795-802: Well-structured dashboard localization hierarchy.The dashboard section has been organized into a logical hierarchy with clear keys for layout, navigation, title, and different categorization methods. This structure will support a more comprehensive and intuitive dashboard interface.
803-803: Added generic items key for better pluralization support.Adding the "items" key allows for proper pluralization in UI elements, which is a good internationalization practice to support different language rules.
804-809: Comprehensive status categorization enhances filtering capabilities.The well-defined status values ("Up to Date", "Needs Review", "Draft", "Empty") provide a clear set of states for evidence items, which will improve filtering and status indicators in the UI.
810-818: Thorough department categorization supports organizational structure.The departments section provides a comprehensive set of department categories that align with common organizational structures in compliance-focused companies. Including an "Uncategorized" option under the "none" key is good practice for handling items without a department assignment.
819-822: Clear separation of review and content sections improves UX organization.The details section clearly separates "Review Information" from "Evidence Content", providing a logical structure for the evidence details view that helps users distinguish between metadata and actual content.
| if (isLoading) { | ||
| return <Skeleton className="h-20 w-full" />; | ||
| } | ||
|
|
||
| if (error) { | ||
| return <div>Error</div>; | ||
| } |
There was a problem hiding this comment.
Remove duplicate loading state handling
The component contains two separate loading state handlers that render different skeleton UIs. The first one (lines 16-18) is more concise, while the second one (lines 28-36) is more detailed but will never be reached since the first condition already returns.
Apply this change to fix the issue:
if (isLoading) {
return (
- <Skeleton className="h-20 w-full" />
+ <div className="flex flex-col gap-4">
+ <Skeleton className="h-20 w-full" />
+ <Skeleton className="h-40 w-full" />
+ <Skeleton className="h-60 w-full" />
+ </div>
);
}
if (error) {
return <div>Error</div>;
}
const handleMutate = async () => {
await mutate();
};
- if (isLoading) {
- return (
- <div className="flex flex-col gap-4">
- <Skeleton className="h-20 w-full" />
- <Skeleton className="h-40 w-full" />
- <Skeleton className="h-60 w-full" />
- </div>
- );
- }Also applies to: 28-36
| if (error) { | ||
| return <div>Error</div>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with error details and retry option
The current error state only displays "Error" without any details or recovery options, which provides a poor user experience.
Improve the error handling with more context and a retry option:
if (error) {
- return <div>Error</div>;
+ return (
+ <div className="flex flex-col gap-4 items-center p-4">
+ <div className="text-red-500 font-medium">Failed to load evidence details</div>
+ <p className="text-sm text-muted-foreground">{error.message || "An unexpected error occurred"}</p>
+ <Button variant="outline" size="sm" onClick={() => mutate()}>
+ Retry
+ </Button>
+ </div>
+ );
}📝 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.
| if (error) { | |
| return <div>Error</div>; | |
| } | |
| if (error) { | |
| return ( | |
| <div className="flex flex-col gap-4 items-center p-4"> | |
| <div className="text-red-500 font-medium">Failed to load evidence details</div> | |
| <p className="text-sm text-muted-foreground">{error.message || "An unexpected error occurred"}</p> | |
| <Button variant="outline" size="sm" onClick={() => mutate()}> | |
| Retry | |
| </Button> | |
| </div> | |
| ); | |
| } |
| // Helper function to get status label | ||
| function getStatusLabel(status: StatusType, t: any): string { | ||
| switch (status) { | ||
| case "upToDate": | ||
| return t("evidence.status.up_to_date"); | ||
| case "needsReview": | ||
| return t("evidence.status.needs_review"); | ||
| case "draft": | ||
| return t("evidence.status.draft"); | ||
| case "empty": | ||
| return t("evidence.status.empty"); | ||
| default: | ||
| return status; | ||
| } | ||
| } | ||
|
|
||
| // Helper function to get status color | ||
| function getStatusColor(status: StatusType): string { | ||
| switch (status) { | ||
| case "upToDate": | ||
| return "#10b981"; // Green | ||
| case "draft": | ||
| return "#f59e0b"; // Amber | ||
| case "needsReview": | ||
| return "#ef4444"; // Red | ||
| case "empty": | ||
| return "#6b7280"; // Gray | ||
| default: | ||
| return "#6b7280"; // Default gray | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY violation: Duplicated helper functions across charts
The getStatusLabel and getStatusColor functions are duplicated across multiple chart components (DepartmentBarChart, FrameworkBarChart, AssigneeBarChart).
Extract these helper functions to a shared utility file to improve maintainability:
// Create a new file: apps/app/src/app/[locale]/(app)/(dashboard)/evidence/overview/components/chartUtils.ts
import type { Translator } from "@/locales/client";
export const STATUS_PRIORITY = ["upToDate", "draft", "needsReview", "empty"] as const;
export type StatusType = (typeof STATUS_PRIORITY)[number];
export const STATUS_COLORS = {
upToDate: "bg-primary",
draft: "bg-[var(--chart-open)]",
needsReview: "bg-[hsl(var(--destructive))]",
empty: "bg-[var(--chart-pending)]",
};
export function getStatusLabel(status: StatusType, t: Translator): string {
switch (status) {
case "upToDate":
return t("evidence.status.up_to_date");
case "needsReview":
return t("evidence.status.needs_review");
case "draft":
return t("evidence.status.draft");
case "empty":
return t("evidence.status.empty");
default:
return status;
}
}
export function getStatusColor(status: StatusType): string {
switch (status) {
case "upToDate":
return "var(--primary)"; // Green
case "draft":
return "var(--chart-open)"; // Amber
case "needsReview":
return "hsl(var(--destructive))"; // Red
case "empty":
return "var(--chart-pending)"; // Gray
default:
return "var(--chart-pending)"; // Default gray
}
}Then import these from each chart component to avoid code duplication.
- Rename Components directory to lowercase components - Update import paths to reflect new directory structure - Reorganize actions and hooks imports - Migrate edit evidence page to use client component with server-side data fetching - Apply consistent formatting and trailing comma rules
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/app/package.json (1)
9-9: Switch from Next.js linter to Biome linterThis change replaces Next.js's built-in linting tool with Biome linter, targeting only the src directory. Biome is a fast formatter and linter with stricter defaults.
Consider adding a
.biome.jsonconfiguration file in the project root to maintain consistent linting rules across the codebase. Also, ensure team members are aware of this tooling change as it might affect their workflow.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/components/EditEvidenceForm.tsx (1)
30-32: Add error handling to mutate functionThe
handleMutatefunction doesn't handle failures of the mutation operation.Consider adding try/catch block to handle potential errors during mutation:
- const handleMutate = async () => { - await mutate(); - }; + const handleMutate = async () => { + try { + await mutate(); + } catch (error) { + toast.error("Failed to update evidence data"); + console.error("Mutation failed:", error); + } + };Don't forget to import the toast component at the top of the file:
import { toast } from "sonner";apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx (1)
93-153: Consider responsive layout for smaller screensThe grid layout with 2 columns works well on desktop but might not be optimal on smaller screens.
Consider using responsive grid columns to ensure a better experience on mobile devices:
- <div className="grid grid-cols-2 gap-8"> + <div className="grid grid-cols-1 md:grid-cols-2 gap-4 md:gap-8">apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (1)
27-29: Consider adding error handling to mutate functionSimilar to the EditEvidenceForm component, the handleMutate function here could benefit from error handling.
- const handleMutate = async () => { - await mutate(); - }; + const handleMutate = async () => { + try { + await mutate(); + } catch (error) { + console.error("Failed to update evidence:", error); + // Consider adding a toast notification here + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/app/.vscode/settings.json(1 hunks)apps/app/package.json(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/AssigneeSection.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/DepartmentSection.tsx(4 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FrequencySection.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx(6 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/components/EditEvidenceForm.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileDelete.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFilePreview.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileUpload.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationAdmins.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationEvidence.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useUrlManagement.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/page.tsx(1 hunks)apps/app/src/middleware.ts(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- apps/app/.vscode/settings.json
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FrequencySection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationEvidence.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/DepartmentSection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileUpload.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useOrganizationAdmins.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/AssigneeSection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFilePreview.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/page.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useUrlManagement.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/page.tsx
- apps/app/src/middleware.ts
🔇 Additional comments (9)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileDelete.ts (2)
46-46: Trailing comma removed from dependency array.The trailing comma in the useCallback dependency array has been removed. This is a minor style change that doesn't affect functionality.
15-47: Implementation looks correct and follows React best practices.The useCallback implementation correctly memoizes the handleDelete function with the appropriate dependencies. The error handling is robust, with specific error messages and appropriate toast notifications for success and failure cases.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/edit/components/EditEvidenceForm.tsx (1)
38-61: LGTM! Card layout structure is well-implementedThe card layout with separate sections for files and URLs is well-structured and provides a good separation of concerns. The component correctly passes the necessary props to child components.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewSection.tsx (4)
4-11: Improved import organizationThe imports have been reorganized for better readability and grouping.
53-61: Well-implemented publish evidence functionalityThe publish evidence action is properly implemented with loading state handling and appropriate success/error toast notifications.
65-91: Better evidence overview presentationThe card header now provides more context with an improved title and helpful subtitle that clearly communicates the purpose of this section.
154-163: Well-implemented conditional publish buttonThe publish button is correctly shown only when the evidence isn't published yet, and it includes proper loading state management.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (2)
13-21: Simplified loading stateThe loading skeleton has been simplified to a more straightforward structure with appropriately sized placeholders.
32-64: Improved layout with Alert componentThe switch from Card to Alert component provides a more appropriate semantic structure for displaying evidence information and its status.
Consider extracting the status indicator into a separate component for better reusability:
+ const PublishStatus = ({ published }: { published: boolean }) => { + return published ? ( + <div className="flex items-center gap-2"> + <CheckCircle2 size={16} className="text-green-500 shrink-0" /> + <span className="text-sm text-green-600">Published</span> + </div> + ) : ( + <div className="flex items-center gap-2"> + <XCircle size={16} className="text-red-500 shrink-0" /> + <span className="text-sm text-red-600">Draft</span> + </div> + ); + };Then use it within the AlertTitle:
<div className="flex items-center gap-2"> <PublishStatus published={evidence.published} /> </div>
| import { useCallback } from "react"; | ||
| import { useToast } from "@bubba/ui/use-toast"; | ||
| import { deleteEvidenceFile } from "../Actions/deleteEvidenceFile"; | ||
| import { deleteEvidenceFile } from "../actions/deleteEvidenceFile"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Directory path casing updated for consistency.
The import path has been updated from ../Actions/deleteEvidenceFile to ../actions/deleteEvidenceFile, changing from uppercase to lowercase. This aligns with common JavaScript/TypeScript conventions for directory naming.
🏁 Script executed:
#!/bin/bash
# Check if both the old (Actions) and new (actions) directories exist
# which could indicate that the directory wasn't actually renamed
echo "Checking directory structure..."
if [ -d "apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions" ] && [ -d "apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/actions" ]; then
echo "WARNING: Both 'Actions' and 'actions' directories exist. This could cause issues."
elif [ -d "apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions" ]; then
echo "WARNING: Only the old 'Actions' directory exists. The import may fail."
elif [ -d "apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/actions" ]; then
echo "SUCCESS: Only the new 'actions' directory exists. The import should work correctly."
else
echo "WARNING: Neither directory exists. The import will fail."
fi
# Check if there are other imports from the old Actions directory that might need updating
echo "Checking for other imports using the old path..."
rg -l "from \".*Actions/" --type tsLength of output: 1276
Action Required: Resolve Import Case Inconsistency
The import in apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/hooks/useFileDelete.ts was changed from
import { deleteEvidenceFile } from "../Actions/deleteEvidenceFile";to
import { deleteEvidenceFile } from "../actions/deleteEvidenceFile";However, the verification script indicates that only the old "Actions" directory exists on disk. This means the current change to lowercase will likely cause the import to fail on case-sensitive filesystems. Additionally, similar imports in other parts of the project still reference the uppercase directory.
Recommendations:
- Revert or Correct: Revert the change in
useFileDelete.tsto use the correct casing (i.e.,"../Actions/deleteEvidenceFile") unless the intended change is to rename the directory. - Consistency Check: If a change to lowercase is desired project-wide, update the filesystem (rename the directory) and modify all affected import paths accordingly.
| if (error) { | ||
| return <div>Error</div>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with more context
The current error state displays a generic message without providing any useful information to the user.
Enhance the error handling by providing more context about what went wrong and possibly offering recovery options:
- if (error) {
- return <div>Error</div>;
- }
+ if (error) {
+ return (
+ <div className="p-4 text-red-600 border border-red-300 rounded-md bg-red-50">
+ <p className="font-medium">Failed to load evidence</p>
+ <p className="text-sm">{error.message || "Please try again or contact support if the issue persists."}</p>
+ </div>
+ );
+ }📝 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.
| if (error) { | |
| return <div>Error</div>; | |
| } | |
| if (error) { | |
| return ( | |
| <div className="p-4 text-red-600 border border-red-300 rounded-md bg-red-50"> | |
| <p className="font-medium">Failed to load evidence</p> | |
| <p className="text-sm"> | |
| {error.message || "Please try again or contact support if the issue persists."} | |
| </p> | |
| </div> | |
| ); | |
| } |
- Update evidence-related translations in es, fr, no, and pt locales - Refactor evidence section with consistent keys and structure - Add new translations for evidence dashboard, status, and departments - Improve language consistency and formatting
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores