Mariano/stuff - #77
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes extend the data model and UI functionality across the application and database layers. New frequency fields are added to organization policies and evidence, while API endpoints are enhanced with refined filtering and pagination. UI components now support additional filters, sorting, and dynamic column resizing. New actions (e.g., for publishing evidence) and utility functions (for calculating review dates) have been introduced. On the database side, new ENUM types and migrations update table schemas. The seeding process and JSON configuration files are also updated to integrate the additional frequency attribute. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ED as EvidenceDetails Component
participant PE as publishEvidence Action
participant DB as Database
participant RTC as ReviewDateCard Component
U->>ED: Click "Publish Evidence" button
ED->>PE: Call publishEvidence action
PE->>DB: Validate user/org and update evidence (set published status, lastPublishedAt, frequency)
DB-->>PE: Return update result
PE-->>ED: Send success response
ED->>RTC: Request updated review information
RTC-->>ED: Render next review date info
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/layout.tsx (1)
2-2: 🛠️ Refactor suggestionRemove unused import.
The
SecondaryMenucomponent is imported but not used in the code.-import { SecondaryMenu } from "@bubba/ui/secondary-menu";
🧹 Nitpick comments (32)
packages/db/prisma/migrations/20250224174548_add_frequency_to_evidence/migration.sql (1)
1-2: New Column Addition in Evidence Table
A new column"frequency"of type"Frequency"is introduced in the"Evidence"table. Ensure that existing records are handled appropriately—if this column is meant to be mandatory, consider specifying a default value or a NOT NULL constraint to avoid potential runtime issues on existing data.packages/data/policies/data_center.json (1)
8-8: Frequency Field Integration in Metadata
The addition of"frequency": "yearly"to the metadata is correctly implemented. One note: the metadata now uses"yearly", while the table cell under "Review Frequency" displays"Annual". Verify that this difference is intentional and that any downstream processing (e.g., mapping to a database ENUM) handles this consistently.packages/data/policies/thirdparty.json (1)
8-8: Frequency Field Addition for Third-Party Policy
The"frequency": "yearly"property is successfully added to the metadata. As with other policies, please check that the usage aligns across the system, especially if there is any logic converting between the metadata value and the display (e.g., "Annual" vs. "yearly").packages/data/policies/data_classification.json (1)
8-8: New Frequency Attribute Added
The"frequency": "yearly"property has been introduced in the metadata section to indicate that the policy should be reviewed on a yearly basis. This update aligns with the broader PR objectives. Please confirm that downstream consumers—such as the UI renderers or database migrations—are aware of this new property, and consider if the internal value"yearly"should be mapped to a display-friendly string (e.g.,"Annual") consistently across the application.packages/data/policies/access_control.json (1)
8-8: Frequency Property Consistency
The addition of"frequency": "yearly"in the metadata is consistent with similar updates across the policy documents. Verify that any code consuming this JSON (e.g., for UI display or integration with database enums) correctly maps this internal value to the intended user-facing label (as seen elsewhere with “Annual”).packages/data/policies/code_of_conduct.json (1)
8-8: Metadata Enhancement with Frequency
The"frequency": "yearly"property is now added to the Code of Conduct Policy metadata, which enhances the document by explicitly defining its review cadence. Ensure that any consumers of this file (for audits or UI rendering) are updated to handle this new field, and that if a display string is required, it should remain consistent with other policies.packages/data/policies/information_security.json (1)
8-8: Review Frequency Field Inclusion
The Information Security Policy now includes"frequency": "yearly"in its metadata, aligning it with the updated data model for review scheduling. It is advisable to review the mapping between these metadata values and the corresponding display values (e.g., “Annual”) in the UI components to ensure a consistent experience.packages/data/policies/cyber_risk.json (1)
8-8: Cyber Risk Policy Frequency Update
The addition of"frequency": "yearly"to the Cyber Risk Assessment Policy metadata ensures that the document now clearly indicates a yearly review cycle. Please ensure that the backend and frontend integration (notably the ENUM type in the database and the UI components) correctly handle this new field and that any references to review frequency reflect the intended mapping (e.g., “yearly” stored vs. “Annual” displayed).apps/app/src/app/[locale]/(app)/(dashboard)/evidence/layout.tsx (1)
9-9: Address the TODO comment regarding i18n implementation.The comment indicates that i18n implementation is pending and was causing build failures.
Would you like me to help implement the i18n functionality or create an issue to track this task?
apps/app/src/lib/utils/calculate-next-review.ts (2)
9-14: Add JSDoc documentation for better code clarity.The function would benefit from comprehensive documentation explaining its purpose, parameters, and return value.
+/** + * Calculates the next review date based on the last published date and frequency. + * @param lastPublishedAt - The date when the item was last published + * @param frequency - The frequency of reviews (monthly, quarterly, or yearly) + * @param urgentThresholdDays - Number of days before review is considered urgent (default: 7) + * @returns ReviewInfo object containing next review date and urgency status, or null if inputs are invalid + */ export function calculateNextReview( lastPublishedAt: Date | null, frequency: Frequency | null, urgentThresholdDays = 7 ): ReviewInfo | null {
12-12: Add input validation for urgentThresholdDays.The function should validate that urgentThresholdDays is a positive number.
urgentThresholdDays = 7 ): ReviewInfo | null { + if (urgentThresholdDays < 0) { + throw new Error('urgentThresholdDays must be a positive number'); + } if (!frequency || !lastPublishedAt) return null;Also applies to: 33-35
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewDateCard.tsx (1)
14-27: Consider adding a more descriptive message for ASAP case.While the current implementation is functional, consider providing more context about why immediate review is needed.
- <p className="text-red-500 font-bold">ASAP</p> + <p className="text-red-500 font-bold"> + ASAP - No review schedule set + </p>apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileSection.tsx (1)
96-113: Consider extracting the Add Files card to a separate component.The Add Files card could be extracted to improve code organization and reusability.
+// AddFilesCard.tsx +interface AddFilesCardProps { + onClick: () => void; +} + +export function AddFilesCard({ onClick }: AddFilesCardProps) { + return ( + <Card + className="group cursor-pointer transition-all hover:shadow-md border-dashed border-2 border-primary/30 hover:border-primary h-[220px] flex flex-col overflow-hidden" + onClick={onClick} + > + <CardContent className="flex flex-col items-center justify-center h-full p-4"> + <div className="rounded-full bg-primary/10 p-3 mb-2"> + <Plus className="h-6 w-6 text-primary" /> + </div> + <p className="text-sm font-medium text-center">Add Files</p> + <p className="text-xs text-muted-foreground mt-1 text-center"> + Upload additional evidence files + </p> + </CardContent> + </Card> + ); +}apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/publishEvidence.ts (1)
42-77: Consider adding transaction for atomic updates.While the current implementation works, wrapping the database operations in a transaction would ensure atomicity.
try { + return await db.$transaction(async (tx) => { // Check if evidence exists and belongs to organization - const evidence = await db.organizationEvidence.findFirst({ + const evidence = await tx.organizationEvidence.findFirst({ where: { id, organizationId: user.organizationId, }, }); if (!evidence) { return { success: false, error: "Evidence not found", }; } // Update the evidence to mark it as published - await db.organizationEvidence.update({ + await tx.organizationEvidence.update({ where: { id }, data: { published: true, lastPublishedAt: new Date(), }, }); return { success: true, }; + }); } catch (error) {apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table.tsx (2)
27-38: Consider adding pagination for large datasets.While sorting is implemented well, consider adding pagination to handle large datasets efficiently.
const table = useReactTable({ data, columns, getCoreRowModel: getCoreRowModel(), getSortedRowModel: getSortedRowModel(), + getPaginationRowModel: getPaginationRowModel(), enableColumnResizing: true, columnResizeMode: "onChange", state: { sorting, + pagination: { + pageIndex: 0, + pageSize: 10, + }, }, onSortingChange: setSorting, + onPaginationChange: setPagination, });
41-93: Consider adding keyboard navigation for better accessibility.While the table implementation is solid, consider enhancing keyboard navigation for better accessibility.
<TableRow key={row.id} data-state={row.getIsSelected() && "selected"} className="hover:bg-muted/50 cursor-pointer" onClick={() => router.push(`/evidence/${row.original.id}`)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + router.push(`/evidence/${row.original.id}`); + } + }} + tabIndex={0} + role="link" >apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx (2)
73-73: Avoid leaving console logs in production code.
console.log({ evidenceTasks, error })can clutter logs. Consider removing or replacing it with more structured logging if needed.
75-87: Reuse smaller helper for unique frequencies for larger datasets.Building a
Setand transforming it viauseMemoworks fine. However, for data with large cardinalities or frequent updates, consider building the unique frequencies in a memoized server-side or store function to keep the UI responsive.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (1)
7-7: Remove unused import if not needed.
CardTitleis imported but not used. Removing unused imports declutters the code and aligns with best practices.-import { Card, CardContent, CardHeader, CardTitle } from "@bubba/ui/card"; +import { Card, CardContent, CardHeader } from "@bubba/ui/card";apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx (3)
54-55: Consider a more fluid height.
Hard-coding a fixed height of 220px could hamper responsiveness on smaller screens. Opting for a more adaptive or auto height can improve the layout’s flexibility.
58-79: Add accessible labeling for the preview button.
This button lacks a text descriptor, potentially making it inaccessible to screen readers. Consider adding anaria-labelor visually hidden text to clarify its purpose.
179-179: Provide feedback on deletion errors.
IfonDeletefails, the user won’t see a status update. Consider adding toast notifications or UI feedback to handle errors gracefully.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table-header.tsx (1)
14-61: Enforce a minimum column width.
Usingheader.getSize() - 32could lead to negative widths if the table shrinks too much. Safeguard against very narrow columns to avoid layout distortion.Increase accessibility for sorting.
Consider applyingaria-sortor an equivalent attribute to inform users of the current sort state. This ensures a better experience for screen reader users.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts (2)
27-30: Pagination defaults are sensible.
A starting page of 1 and a page size of 10 are standard. Consider bounding the page size with an upper limit to avoid excessive queries.
52-87: Potential performance consideration withORsearches.
Using multipleORconditions can degrade performance if data grows large. Evaluate whether partial indexes or a more advanced search approach (e.g., full-text indexing) might be beneficial long-term.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx (1)
50-53: Consider adding error handling for clipboard operations.The copyToClipboard function should handle potential failures when writing to clipboard.
-const copyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); +const copyToClipboard = async (text: string) => { + try { + await navigator.clipboard.writeText(text); + } catch (error) { + console.error('Failed to copy text:', error); + } +};packages/db/prisma/seed.ts (1)
388-405: Improve variable naming consistency.The variable name change from
evidencetoevidenceReqimproves clarity, but consider using full words.-for (const evidenceReq of evidenceRequirements) { +for (const evidenceRequirement of evidenceRequirements) {packages/db/prisma/schema.prisma (1)
1048-1052: Consider adding a default frequency value.The frequency field in OrganizationEvidence could benefit from a default value to ensure consistent behavior.
-frequency Frequency? +frequency Frequency? @default(yearly)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/hooks/useEvidenceTasks.ts (1)
8-15: Consider validating pagination parameters.
Although the interface is well-defined, watch out for invalid pagination values. You might want to validate or guard against negative or zero page/pageSize values.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx (3)
19-33: Check default and minimum column sizing conflict.
You setsize: 100andminSize: 200. TheminSizewill always override the smaller defaultsize. Consider aligning them to avoid layout confusion.
64-78: Consider user-friendly rendering for frequency.
Iffrequencyis an enum, consider mapping it to friendlier labels for readability.
79-108: Improve date localization.
UsingtoLocaleDateString()is acceptable, but you might want a more robust library or i18n approach (e.g., dayjs or date-fns) if you need consistent formatting across locales.
📜 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 (48)
apps/app/src/actions/framework/select-frameworks-action.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table-header.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/publishEvidence.ts(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/FileCard.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileSection.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewDateCard.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/hooks/useEvidenceTasks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/layout.tsx(1 hunks)apps/app/src/lib/utils/calculate-next-review.ts(1 hunks)packages/data/controls/soc2.json(92 hunks)packages/data/policies/access_control.json(1 hunks)packages/data/policies/application_security.json(1 hunks)packages/data/policies/availability.json(1 hunks)packages/data/policies/business_continuity.json(1 hunks)packages/data/policies/change_management.json(1 hunks)packages/data/policies/classification.json(1 hunks)packages/data/policies/code_of_conduct.json(1 hunks)packages/data/policies/confidentiality.json(1 hunks)packages/data/policies/corporate_governance.json(1 hunks)packages/data/policies/cyber_risk.json(1 hunks)packages/data/policies/data_center.json(1 hunks)packages/data/policies/data_classification.json(1 hunks)packages/data/policies/disaster_recovery.json(1 hunks)packages/data/policies/human_resources.json(1 hunks)packages/data/policies/incident_response.json(1 hunks)packages/data/policies/information_security.json(1 hunks)packages/data/policies/password_policy.json(1 hunks)packages/data/policies/privacy.json(1 hunks)packages/data/policies/risk_assessment.json(1 hunks)packages/data/policies/risk_management.json(1 hunks)packages/data/policies/software_development.json(1 hunks)packages/data/policies/system_change.json(1 hunks)packages/data/policies/thirdparty.json(1 hunks)packages/data/policies/vendor_risk_management.json(1 hunks)packages/data/policies/workstation.json(1 hunks)packages/db/prisma/migrations/20250224173509_add_frequency_to_policy_and_evidence/migration.sql(1 hunks)packages/db/prisma/migrations/20250224174548_add_frequency_to_evidence/migration.sql(1 hunks)packages/db/prisma/migrations/20250224180117_add_frequency_to_policy/migration.sql(1 hunks)packages/db/prisma/migrations/20250224183431_add_frequency_to_control_requirement/migration.sql(1 hunks)packages/db/prisma/schema.prisma(6 hunks)packages/db/prisma/seed.ts(8 hunks)packages/db/prisma/seedTypes.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/db/prisma/migrations/20250224183431_add_frequency_to_control_requirement/migration.sql
- packages/db/prisma/migrations/20250224180117_add_frequency_to_policy/migration.sql
- packages/data/policies/password_policy.json
🔇 Additional comments (56)
packages/data/policies/classification.json (1)
8-8: Addition of "frequency" Metadata Property
The"frequency": "yearly"property has been added to the metadata section. This change aligns with similar updates in other policy files and matches the ENUM values defined in the migration scripts. Please verify that"yearly"is the correct review frequency for this policy.packages/data/policies/human_resources.json (1)
8-8: Frequency Field Addition in Human Resources Policy
The new field"frequency": "yearly"in the Human Resources policy metadata ensures consistency with the updated review cycle across all policies. Confirm that this annual frequency meets internal review guidelines.packages/data/policies/availability.json (1)
8-8: Metadata Frequency Update for Availability Policy
The insertion of"frequency": "yearly"in the Availability Policy metadata is consistent with the broader changes across policy documents. Please verify that the annual review cycle specified here complies with any regulatory requirements or internal standards for system availability reviews.packages/data/policies/risk_assessment.json (1)
8-8: Insertion of "frequency" in Risk Assessment Policy Metadata
The new"frequency": "yearly"property signifies that risk assessments are to be conducted on an annual basis. Ensure that this frequency is consistent with your organization's risk management strategy and that any impacted processes or UI components reflect this update.packages/data/policies/privacy.json (1)
8-8: New Frequency Property in Privacy Policy
The new"frequency": "yearly"field is properly integrated into the privacy policy’s metadata, matching the approach taken in the other policy documents. Ensure that this term is consistent with any backend constraints or ENUM values defined in the database migrations.packages/data/policies/confidentiality.json (1)
8-8: Frequency Metadata in Confidentiality Policy
The"frequency": "yearly"property has been added as expected. Please confirm that this value is consistent with the definitions in the database schema (e.g., theFrequencyENUM) and that the user interface correctly interprets this metadata.packages/data/policies/application_security.json (1)
8-8: Integration of Frequency Field in Application Security Policy
The update introducing"frequency": "yearly"into the metadata is well implemented. As a follow-up, verify that any code consuming this data (for filtering or display) uses the proper mapping between the internal value (e.g.,"yearly") and the user-facing label (e.g.,"Annual").packages/data/policies/software_development.json (1)
8-8: Ensure Consistent Policy Metadata
The addition of"frequency": "yearly"in the metadata is clear and consistent with the new review cadence requirements. Verify that any downstream mapping (e.g., to UI labels or database ENUM values) properly interprets"yearly"(which may be displayed as "Annual" elsewhere).packages/data/policies/business_continuity.json (1)
8-8: Validate Frequency Field Integration
The new"frequency": "yearly"property is correctly added to the business continuity policy metadata. Confirm that this value aligns with UI representations and database schema mappings (e.g., the corresponding ENUM) to maintain consistency across the platform.packages/data/policies/system_change.json (1)
8-8: Consistent Frequency Specification for System Change Policy
The addition of"frequency": "yearly"meets the new policy review requirements. As with other policy files, please ensure that the UI and database layers consistently interpret this value (e.g., mapping "yearly" to a display label like "Annual").packages/data/policies/workstation.json (1)
8-8: Appropriate Frequency Field Addition
The new"frequency": "yearly"field in the workstation policy metadata appropriately specifies the review cadence. Make sure that any components (such as table displays showing "Review Frequency") correctly reflect this metadata and that the value maps to the expected ENUM in the database.packages/data/policies/incident_response.json (1)
8-8: Clear Addition of Frequency for Incident Response
The inclusion of"frequency": "yearly"in the incident response policy metadata is straightforward and aligns with the broader update. Confirm that this field is integrated into downstream processes—such as policy review scheduling, UI displays, and database schema updates—to ensure end-to-end consistency.packages/data/policies/risk_management.json (1)
8-8: Addition of "frequency" Property
The new"frequency": "yearly"property is correctly added to the metadata section. This value aligns with the ENUM values defined in the database schema (“monthly”, “quarterly”, “yearly”), ensuring consistency across policy documents.packages/data/policies/change_management.json (1)
8-8: Introducing the Frequency Attribute
The addition of"frequency": "yearly"enhances the metadata by explicitly defining the review interval. This change is consistent with similar updates across other policies and supports subsequent database integration.packages/data/policies/vendor_risk_management.json (1)
8-8: Consistent Frequency Field Update
Including"frequency": "yearly"clearly communicates that the vendor risk management policy is to be reviewed annually. This update mirrors changes in other policy files and fits within the overall design for policy frequency management.packages/data/policies/corporate_governance.json (1)
8-8: New Frequency Attribute Added
The new"frequency": "yearly"property effectively sets the review cadence for the corporate governance policy. Its usage is consistent with the broader changes across policy documents and the expected database ENUM values.packages/data/policies/disaster_recovery.json (1)
8-8: Disaster Recovery Frequency Update
The addition of"frequency": "yearly"improves clarity regarding the review interval for the disaster recovery policy. This update aligns with similar modifications in other policy documents and adheres to the standardized frequency values.apps/app/src/actions/framework/select-frameworks-action.ts (1)
152-152:Details
❓ Verification inconclusive
Consider handling null frequency values.
The frequency values from
policy.frequencyandevidence.frequencymight be null. Consider providing a default value or handling null cases explicitly.Also applies to: 269-269
🏁 Script executed:
#!/bin/bash # Check if there are any policies or evidence with null frequency values ast-grep --pattern 'frequency: null'Length of output: 38
Potential Null Handling for Frequency Values
In
apps/app/src/actions/framework/select-frameworks-action.ts(line 152), thefrequencyproperty is set directly frompolicy.frequencywithout any explicit null check. Since bothpolicy.frequencyand its counterpart increateOrganizationEvidencemight benull, please ensure that you explicitly handle these cases—either by providing a default value (e.g., using the nullish coalescing operator likepolicy.frequency ?? defaultFrequencyValue) or by adding proper null-check logic—to prevent unexpected runtime behavior.Please verify that this change is compatible with the rest of the codebase and that similar precautions are applied wherever
frequencyvalues are used.packages/db/prisma/seedTypes.ts (1)
1-1: LGTM! Interface updates are consistent with schema changes.The addition of the
frequencyfield to bothRequirementandPolicyinterfaces aligns well with the database schema updates.Also applies to: 21-21, 50-50
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewDateCard.tsx (2)
7-13: LGTM! Props interface is well-defined.The component's props are properly typed with explicit null handling for both
lastPublishedAtandfrequency.
29-42: LGTM! Good use of conditional styling for urgency.The implementation effectively uses conditional styling to highlight urgent reviews, and properly formats the date display.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileSection.tsx (2)
83-94: LGTM! Clean implementation of conditional file upload.The implementation elegantly handles the visibility of the upload dropzone based on both file count and user interaction.
115-141: LGTM! Well-structured grid layout with proper key usage.The implementation uses a responsive grid layout and properly handles file previews and dialog states.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/publishEvidence.ts (1)
7-9: LGTM! Good use of Zod for input validation.The schema definition is clear and type-safe.
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table.tsx (1)
20-25: LGTM! Good default sorting configuration.The initial sorting state is well-defined with a sensible default.
packages/db/prisma/migrations/20250224173509_add_frequency_to_policy_and_evidence/migration.sql (2)
1-2: Define a default or handle NULL constraints for the enum.Creating an enum "Frequency" is fine, but ensure you have a clear strategy for how existing records handle the new column, especially if it's not nullable. Consider adding a default value or making the column nullable to avoid migration failures.
9-10: Mirror constraints or defaults for "OrganizationPolicy".Same concern applies to the new columns in "OrganizationPolicy". Verify whether existing records require an explicit default "frequency" and whether
lastPublishedAtexpects backfilling or a default.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/EvidenceList.tsx (2)
41-46: Good addition of new state and hook parameters.Your approach for storing
status,frequency,page, andpageSizein query states and passing them touseOrganizationEvidenceTasksis logically consistent. This ensures modular and reactive URL-driven filtering and pagination.Also applies to: 53-62
89-95: Clarify whether “clearFilters” should also reset the search query.While clearing
statusandfrequency, you might also want to reset thesearchquery for complete filter clearing. If this behavior is intentional, document it explicitly.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/EvidenceDetails.tsx (3)
22-30: Efficient publish action with clear feedback.Using
useActionforpublishEvidencepaired with success/error toast messages is a well-structured approach. It provides clear user feedback and a concise implementation. Good job!
70-85: Seamless publish button in the header.Conditionally showing the publish button only when
!evidence.publishedavoids redundant interactions. The disabled state during publishing is a neat UX detail. This helps maintain clarity and consistency.
88-93: Informative “ReviewDateCard” usage.Displaying the last published date with
frequencyprovides transparency to the user. EnsureReviewDateCardcorrectly handles edge cases wherelastPublishedAtmight be null.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/FileCard.tsx (4)
117-118: Footer layout changes look good.
The new classes cohesively align elements at the bottom without introducing overflow issues.
122-142: Verify fallback whenpreviewState.urlis null.
You cleanly prevent the link’s default action ifpreviewState.urlis absent, but users might be left uncertain. Consider showing a disabled state or a message explaining that a preview is unavailable.
145-145: Tooltip usage is consistent.
“Open file” is an appropriate label, aligning well with the existing UI patterns.
152-169: Deletion logic is well-implemented.
The outlined destructive variant with a confirmation dialog follows recommended UI patterns.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/data-table-header.tsx (3)
5-5: New import is straightforward.
ImportingflexRenderaligns with the TanStack Table approach. No issues found.
7-8: Icon imports and class merging utility.
Pulling in arrow icons and thecnhelper is consistent with project conventions.
10-12: Interface definition clarifies prop usage.
DefiningDataTableHeaderPropsimproves readability and maintainability by clearly stating expected properties.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Actions/getOrganizationEvidenceTasks.ts (6)
4-5: Imports look appropriate.
Bringing indb,Frequency, and additional types from@bubba/dbmeets the new functionality’s requirements.
8-21: Type definitions add clarity.
PaginationMetadataandEvidenceTasksDatarefine the response structure, enhancing type safety and developer insight.
42-42: Destructured inputs appear correct.
Status, frequency, page, and pageSize are properly extracted and used later in the query.
89-92: Accurate total count retrieval.
Queryingcountfor pagination is a standard approach. This is correctly aligned with the providedwhereClause.
94-99: Pagination calculation is correct.
skipandtakelogic is clean, avoiding off-by-one errors. Ensure the page does not exceed the total number of pages.
100-123: Concise data encapsulation with pagination metadata.
Including pagination details alongsideevidenceTaskskeeps the API uniform and clear. No issues noted with the final return structure.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/UrlSection.tsx (3)
41-48: LGTM! Well-implemented domain extraction function.The getDomainName function correctly handles URL parsing and gracefully falls back to the original URL on error.
192-200: LGTM! Secure external link implementation.The external link implementation correctly uses
rel="noopener noreferrer"for security.
164-225:Details
❓ Verification inconclusive
Verify accessibility of tooltip buttons.
The buttons with tooltips need proper ARIA labels for screen readers.
🌐 Web query:
What are the WCAG requirements for tooltip accessibility?💡 Result:
To ensure tooltips meet WCAG accessibility standards, they must adhere to specific requirements under Success Criterion 1.4.13 (Content on Hover or Focus)[8][10]. These guidelines focus on making tooltips perceivable and operable for all users, including those using assistive technologies.
Key Requirements for Accessible Tooltips
1. Dismissible
- Tooltips must be closable without moving the mouse or changing focus (e.g., via the Escape key)[3][6][8].
- Avoid auto-dismissing content unless it becomes irrelevant (e.g., a timeout)[8].
2. Hoverable
- Content must remain visible when users move their pointer over it[6][8].
- Example: A tooltip should stay open if a user hovers over its text to read it[8].
3. Persistent
- Tooltips must stay visible until dismissed or until the trigger is removed (e.g., moving focus away)[6][8].
- Avoid designs where tooltips disappear abruptly[8].
Technical Implementation Guidelines
Semantic Structure
- Use
role="tooltip"for the tooltip container[3][7].- Link the tooltip to its trigger element with
aria-describedby[3][6].Keyboard Navigation
- Ensure tooltips can be triggered via keyboard focus (e.g., using
tabindex="0"on non-focusable elements)[3].- Support closing with the Escape key[3][6].
Visual Design
- Contrast: Meet WCAG’s minimum contrast ratio of 4.5:1 between text and background[4].
- Positioning: Avoid obscuring related content[2][6]. Use arrows to clarify associations if multiple elements are nearby[2].
- Typography: Use clear fonts and adequate size for readability[1][4].
Content Guidelines
- Keep text concise (avoid long paragraphs)[1][2].
- Avoid critical information in tooltips; use visible labels instead[2][7].
Common Pitfalls to Avoid
- Non-dismissible content: Failing to provide an Escape key option[8].
- Lack of keyboard support: Tooltips only triggered by mouse hover[2][6].
- Low contrast: Text that blends into the background[4].
- Blocking content: Tooltips covering essential UI elements[2][6].
Tools & Testing
- Validate compliance using tools like WAVE or axe-core[5].
- Test with screen readers (e.g., NVDA, VoiceOver) and keyboard navigation[5][6].
By adhering to these guidelines, tooltips become inclusive for users with disabilities while aligning with legal standards like ADA and WCAG 2.1 AA[4][5].
Citations:
- 1: https://www.a11y-collective.com/blog/tooltips-in-web-accessibility/
- 2: https://www.nngroup.com/articles/tooltip-guidelines/
- 3: https://theadminbar.com/accessibility-weekly/accessible-tooltips/
- 4: https://userway.org/blog/your-guide-to-accessible-tooltips/
- 5: https://designsystem.digital.gov/components/tooltip/accessibility-tests/
- 6: https://sarahmhigley.com/writing/tooltips-in-wcag-21/
- 7: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tooltip_role
- 8: https://www.wcag.com/authors/1-4-13-content-on-hover-or-focus/
- 9: https://www.w3.org/TR/WCAG21/
- 10: https://www.w3.org/WAI/WCAG22/Understanding/content-on-hover-or-focus.html
Accessibility Improvement Required: Ensure Tooltip Buttons Are Properly Labeled
Please verify that each tooltip automatically associates the trigger button with its tooltip content using the appropriate ARIA attributes (such as
aria-describedbyon the button androle="tooltip"on the tooltip content). If these aren’t handled internally by the tooltip components, add them to meet WCAG Success Criterion 1.4.13 guidelines. Also, ensure tooltips remain keyboard accessible (e.g., dismissible via the Escape key).packages/db/prisma/seed.ts (1)
23-24: LGTM! Improved database cleanup sequence.The cleanup sequence is well-organized, ensuring proper deletion order to avoid foreign key constraint issues.
Also applies to: 36-36
packages/db/prisma/schema.prisma (1)
41-45: LGTM! Well-defined frequency enum.The Frequency enum values are appropriately defined for the use case.
packages/data/controls/soc2.json (1)
14-15: LGTM! Appropriate frequency assignments.The frequency values are appropriately assigned based on the control type - yearly for policies and quarterly for critical evidence.
Also applies to: 27-28
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/hooks/useEvidenceTasks.ts (3)
5-6: Typed imports look good.
These imports appear correct and align well with the rest of the file.
17-24: Hook result type is clear and consistent.
Defining and exporting the result interface keeps things readable and coherent, particularly for SWR usage.
46-66: SWR integration and destructuring look good.
After correcting the returned data infetchEvidenceTasks, the use of SWR keys and the returned fields (likedata,pagination, etc.) should work smoothly.apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx (2)
12-12: Ensure test coverage forcalculateNextReview.
The utility function import looks fine. Just verify that any date calculations and edge cases (e.g., frequency undefined) have corresponding tests.
38-41: Column sizing refinements for Status.
These settings (resizing, sorting, size, minSize) and className adjustments look consistent. No issues here.Also applies to: 47-47
| const baseDate = new Date(lastPublishedAt); | ||
| const nextReviewDate = new Date(baseDate); | ||
|
|
||
| switch (frequency) { | ||
| case "monthly": | ||
| nextReviewDate.setMonth(nextReviewDate.getMonth() + 1); | ||
| break; | ||
| case "quarterly": | ||
| nextReviewDate.setMonth(nextReviewDate.getMonth() + 3); | ||
| break; | ||
| case "yearly": | ||
| nextReviewDate.setFullYear(nextReviewDate.getFullYear() + 1); | ||
| break; | ||
| default: | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle timezone-related edge cases in date calculations.
The current date calculations might lead to inconsistencies across different timezones. Consider using UTC dates for consistency.
- const baseDate = new Date(lastPublishedAt);
- const nextReviewDate = new Date(baseDate);
+ const baseDate = new Date(lastPublishedAt.toISOString());
+ const nextReviewDate = new Date(baseDate.toISOString());Committable suggestion skipped: line range outside the PR's diff.
| .action(async ({ parsedInput, ctx }): Promise<ActionResponse> => { | ||
| const { user } = ctx; | ||
| const { id } = parsedInput; | ||
|
|
||
| if (!user.organizationId) { | ||
| return { | ||
| success: false, | ||
| error: "Not authorized - no organization found", | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider adding organization role check.
While the organization check is good, consider also verifying if the user has the appropriate role/permissions to publish evidence.
🏁 Script executed:
#!/bin/bash
# Check if role-based authorization is implemented elsewhere
rg -A 5 "organizationRole|userRole|canPublish|hasPermission"Length of output: 62
Action Required: Add Role-Based Authorization Check
The current implementation verifies that the user is associated with an organization via user.organizationId, but it does not check if the user has the necessary role or permissions (e.g. a publish permission flag or an organizational role) to execute the publish action. Please consider adding a role/permission check—for example, ensuring that the user holds the appropriate role—before proceeding with the publish logic.
- Location:
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Actions/publishEvidence.ts(lines 31–40) - Suggestion: After verifying that
user.organizationIdexists, add an additional condition to check for the required role/permission (such asuser.role,organizationRole, or a specifichasPermissionflag).
| ALTER TABLE "OrganizationEvidence" ADD COLUMN "frequency" "Frequency", | ||
| ADD COLUMN "lastPublishedAt" TIMESTAMP(3); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for potential data conflicts or backfill strategy.
When adding new columns to "OrganizationEvidence", consider a backfill if you intend for existing rows to carry a valid value for "frequency" by default. Similarly, decide whether lastPublishedAt should default to the current timestamp or remain nullable.
| async function fetchEvidenceTasks(props: UseOrganizationEvidenceTasksProps) { | ||
| const result = await getOrganizationEvidenceTasks(props); | ||
|
|
||
| if (!result) { | ||
| throw new Error("No result received"); | ||
| } | ||
|
|
||
| if (!result || "error" in result) { | ||
| if (result.serverError) { | ||
| throw new Error(result.serverError); | ||
| } | ||
|
|
||
| if (result.validationErrors) { | ||
| throw new Error( | ||
| typeof result?.error === "string" | ||
| ? result.error | ||
| : "Failed to fetch evidence tasks" | ||
| result.validationErrors._errors?.join(", ") ?? "Unknown error" | ||
| ); | ||
| } | ||
|
|
||
| return result.data?.data; | ||
| } |
There was a problem hiding this comment.
Return the entire result object to preserve pagination.
Currently, fetchEvidenceTasks returns only result.data?.data but the hook later references pagination. This mismatch will cause pagination to be undefined.
Please adjust line 43 to ensure you return the entire data payload:
- return result.data?.data;
+ return result.data;📝 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.
| async function fetchEvidenceTasks(props: UseOrganizationEvidenceTasksProps) { | |
| const result = await getOrganizationEvidenceTasks(props); | |
| if (!result) { | |
| throw new Error("No result received"); | |
| } | |
| if (!result || "error" in result) { | |
| if (result.serverError) { | |
| throw new Error(result.serverError); | |
| } | |
| if (result.validationErrors) { | |
| throw new Error( | |
| typeof result?.error === "string" | |
| ? result.error | |
| : "Failed to fetch evidence tasks" | |
| result.validationErrors._errors?.join(", ") ?? "Unknown error" | |
| ); | |
| } | |
| return result.data?.data; | |
| } | |
| async function fetchEvidenceTasks(props: UseOrganizationEvidenceTasksProps) { | |
| const result = await getOrganizationEvidenceTasks(props); | |
| if (!result) { | |
| throw new Error("No result received"); | |
| } | |
| if (result.serverError) { | |
| throw new Error(result.serverError); | |
| } | |
| if (result.validationErrors) { | |
| throw new Error( | |
| result.validationErrors._errors?.join(", ") ?? "Unknown error" | |
| ); | |
| } | |
| return result.data; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx (2)
73-78: Improve empty state handling for better UX.While the code correctly handles null frequency values, it doesn't provide any visual indication to users about why the cell is empty.
Apply this diff to improve empty state handling:
cell: ({ row }) => { const frequency = row.original.frequency; - if (!frequency) return null; + if (!frequency) { + return ( + <TooltipProvider> + <Tooltip> + <TooltipTrigger asChild> + <div className="text-gray-400">-</div> + </TooltipTrigger> + <TooltipContent>No frequency set</TooltipContent> + </Tooltip> + </TooltipProvider> + ); + } return <div className="truncate">{frequency}</div>; },
88-108: Use locale-aware date formatting.The date format is hardcoded to "MM/dd/yyyy", which might not be appropriate for all locales. Since the application appears to support internationalization (based on the
[locale]route segment), consider using locale-aware date formatting.Apply this diff to improve date formatting:
-import { format } from "date-fns"; +import { format } from "date-fns-tz"; +import { useParams } from "next/navigation"; export const columns: ColumnDef<EvidenceTaskRow>[] = [ // ... other columns ... { id: "nextReviewDate", // ... other properties ... cell: ({ row }) => { + const { locale } = useParams(); if (row.original.lastPublishedAt === null) { return <div className="truncate text-red-500 font-medium">ASAP</div>; } const reviewInfo = calculateNextReview( row.original.lastPublishedAt, row.original.frequency ); if (!reviewInfo) return null; return ( <div className={`truncate ${reviewInfo.isUrgent ? "text-red-500" : ""}`} > {reviewInfo.daysUntil} days ( - {format(reviewInfo.nextReviewDate, "MM/dd/yyyy")}) + {format(reviewInfo.nextReviewDate, "P", { locale })}) </div> ); }, }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewDateCard.tsx(1 hunks)apps/app/src/lib/utils/calculate-next-review.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/app/src/lib/utils/calculate-next-review.ts
- apps/app/src/app/[locale]/(app)/(dashboard)/evidence/[id]/Components/ReviewDateCard.tsx
🔇 Additional comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/evidence/Components/data-table/columns.tsx (1)
39-42: LGTM!The column configuration is well-structured with consistent size properties and proper enablement of resizing and sorting features.
| enableResizing: true, | ||
| enableSorting: true, | ||
| size: 100, | ||
| minSize: 200, |
There was a problem hiding this comment.
Fix inconsistent size properties.
The column's size property (100) is smaller than its minSize property (200), which means the column will always be at least 200px wide, making the size setting ineffective.
Apply this diff to fix the size properties:
- size: 100,
+ size: 200,
minSize: 200,📝 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.
| enableResizing: true, | |
| enableSorting: true, | |
| size: 100, | |
| minSize: 200, | |
| enableResizing: true, | |
| enableSorting: true, | |
| size: 200, | |
| minSize: 200, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/app/src/app/[locale]/providers.tsx (1)
28-38: LGTM! Clean analytics initialization.The declarative approach using conditional rendering is cleaner than the previous imperative initialization. However, consider improving type safety by removing non-null assertions.
Apply this diff to remove non-null assertions:
{hasAnalyticsKeys ? ( <AnalyticsProvider - apiKey={env.NEXT_PUBLIC_POSTHOG_KEY!} - apiHost={env.NEXT_PUBLIC_POSTHOG_HOST!} + apiKey={env.NEXT_PUBLIC_POSTHOG_KEY} + apiHost={env.NEXT_PUBLIC_POSTHOG_HOST} userId={session?.user?.id} > {children} </AnalyticsProvider> ) : ( children )}packages/analytics/src/index.ts (1)
5-25: Consider adding error handling and type safety.The implementation looks good with proper server-side safety checks. Consider these improvements:
- Add error handling for PostHog method calls
- Add type safety for event names to prevent typos
Example implementation:
+// Define allowed event names +type EventName = "$pageview" | "custom_event_1" | "custom_event_2"; export const Analytics = { - track: (eventName: string, properties?: Properties) => { + track: (eventName: EventName, properties?: Properties) => { if (typeof window === "undefined") return; - posthog.capture(eventName, properties); + try { + posthog.capture(eventName, properties); + } catch (error) { + console.error('Failed to track event:', error); + } }, // Apply similar changes to other methods... };packages/analytics/src/components/provider.tsx (1)
23-31: Consider adding error handling and type safety.The PostHog initialization is well-structured and follows best practices. However, consider these improvements:
- Add error handling for the initialization
- Add type safety for the loaded callback
// Initialize PostHog only once on the client side -posthog.init(apiKey, { - api_host: apiHost, - loaded: (ph) => { - if (userId) { - ph.identify(userId); - } - }, -}); +try { + posthog.init(apiKey, { + api_host: apiHost, + loaded: (ph: typeof posthog) => { + if (userId) { + ph.identify(userId); + } + }, + }); +} catch (error) { + console.error('Failed to initialize PostHog:', error); +}packages/analytics/src/components/page-view.tsx (1)
12-25: Consider enhancing URL construction robustness.While the current implementation works, we could make it more resilient and maintainable.
Consider this refactoring:
useEffect(() => { if (pathname && posthog) { // Track page views - const url = searchParams.toString() - ? `${pathname}?${searchParams.toString()}` - : pathname; + const getPageUrl = () => { + const params = searchParams.toString(); + return params && searchParams.size > 0 + ? `${pathname}?${params}` + : pathname; + }; // Track pageview with current URL posthog.capture("$pageview", { - $current_url: url, + $current_url: getPageUrl(), }); } }, [pathname, searchParams, posthog]);This refactoring:
- Adds an explicit check for empty searchParams using
.size- Improves readability by extracting URL construction logic
- Makes the code more maintainable and testable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/app/src/app/[locale]/providers.tsx(1 hunks)packages/analytics/src/components/page-view.tsx(1 hunks)packages/analytics/src/components/provider.tsx(1 hunks)packages/analytics/src/index.ts(1 hunks)
🔇 Additional comments (9)
apps/app/src/app/[locale]/providers.tsx (1)
5-5: LGTM! Clean import changes.The removal of the
Analyticsimport while retainingAnalyticsProvideraligns well with the shift to a more declarative analytics initialization approach.packages/analytics/src/index.ts (2)
1-4: Great architectural simplification!The removal of the initialization complexity and direct usage of PostHog methods makes the implementation more maintainable while ensuring server-side safety.
27-29: LGTM!The export statements are well-organized and clearly documented.
packages/analytics/src/components/provider.tsx (2)
21-21: LGTM! Good documentation.The comment clearly explains why
useEffectis used for PostHog initialization, which is important for understanding hydration-related concerns in React Server Components.
33-36: LGTM! Proper cleanup implementation.The cleanup logic is correct and the removal of optional chaining is appropriate since PostHog is guaranteed to be initialized at this point.
packages/analytics/src/components/page-view.tsx (4)
1-6: LGTM! Clean import setup.The imports are well-organized and include all necessary dependencies for PostHog page view tracking.
7-11: LGTM! Well-structured component setup.The component is properly exported and uses the necessary hooks for page tracking functionality.
27-28: LGTM! Appropriate return value.Correctly returns
nullas this is a tracking-only component with no UI elements.
1-28:Details
✅ Verification successful
Verify integration with dependent components.
The AI summary indicates this component is used by evidence-related components. Let's verify the integration is not broken.
🏁 Script executed:
#!/bin/bash # Search for usages of both old and new component names echo "Searching for old PageViewTracker usage..." rg "PageViewTracker" echo -e "\nSearching for new PostHogPageView usage..." rg "PostHogPageView"Length of output: 755
Integration Verified: No further changes required
The search confirmed that there are no remaining references to the deprecated
PageViewTracker, and all usages have been correctly updated toPostHogPageView. The new component is referenced in the following locations:
apps/web/src/app/components/posthog-pageview.tsx(both in the named and default export asSuspendedPostHogPageView)packages/analytics/src/components/page-view.tsxpackages/analytics/src/components/provider.tsxThis indicates proper integration with dependent components, so you can proceed with confidence.
This branch was successfully deployed
1 participant
Summary by CodeRabbit