Mariano/evidence improvements - #244
Conversation
…, updating related components, and enhancing data handling for improved clarity and functionality
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes update evidence management across the codebase. They include cosmetic reformatting in utility functions, renaming and restructuring of evidence status constants and types, and enhanced filtering logic using descriptive status strings. New server actions and modular UI components—handling evidence assignment, department selection, frequency, next review, and status—replace older implementations. Major database and Prisma schema migrations were also applied, dropping obsolete columns and updating relationships to support the new evidence status enum and member associations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant EP as Evidence Page
participant ED as EvidenceDetails/ReviewSection UI
participant AE as updateEvidenceDetails Action
participant DB as Database
U->>ED: Updates evidence details (status, assignee, frequency, etc.)
ED->>AE: Submits updated details
AE->>DB: Validate session & fetch evidence record
DB-->>AE: Return evidence record
AE->>DB: Update record with new details
DB-->>AE: Confirmation of update
AE-->>ED: Return success response
ED-->>U: Display updated evidence information
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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)/[orgId]/evidence/list/components/table/EvidenceListColumns.tsx (1)
43-45: 🛠️ Refactor suggestionUpdate accessorKey to match the new status field
The column definition still uses
accessorKey: "published"while the implementation now usesrow.original.status. This inconsistency should be fixed to maintain proper column sorting and filtering capabilities.{ id: "status", - accessorKey: "published", + accessorKey: "status", header: "Status",
🧹 Nitpick comments (17)
packages/db/prisma/migrations/20250403153215_idk/migration.sql (1)
1-2: Consider using a more descriptive migration nameThe migration correctly drops the index on
Evidence_assigneeId_idx, which makes sense given the changes to the Evidence model's relationships. However, the migration filename contains "idk" which isn't descriptive of its purpose.For better maintainability, consider using more descriptive names for migrations such as "drop_evidence_assignee_index" to clearly indicate the purpose of the migration.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceNextReviewSection.tsx (2)
21-30: Enhance date display and localizationThe review date display could be improved in several ways:
- Consider using localized date formatting rather than hardcoded "MM/dd/yyyy"
- Add handling for special cases:
- When daysUntil = 1 (use "day" instead of "days")
- When daysUntil = 0 (show "Today" instead of "0 days")
- When daysUntil < 0 (show "Overdue" or similar)
- The text "ASAP" and "days" should be localized
Here's a suggested improvement:
{!reviewInfo ? ( - <p className="text-red-500 font-medium text-sm">ASAP</p> + <p className="text-red-500 font-medium text-sm">{t('evidence.review.asap')}</p> ) : ( <div className={`text-sm font-medium ${reviewInfo.isUrgent ? "text-red-500" : ""}`} > - {reviewInfo.daysUntil} days ( - {format(reviewInfo.nextReviewDate, "MM/dd/yyyy")}) + {reviewInfo.daysUntil < 0 + ? t('evidence.review.overdue', { days: Math.abs(reviewInfo.daysUntil) }) + : reviewInfo.daysUntil === 0 + ? t('evidence.review.today') + : t('evidence.review.dayCount', { + count: reviewInfo.daysUntil, + days: reviewInfo.daysUntil === 1 ? t('evidence.review.day') : t('evidence.review.days') + }) + } ({format(reviewInfo.nextReviewDate, 'P')}) </div> )}This assumes the existence of a translation function
t(). If you're not using a localization library yet, consider integrating one likereact-intlornext-i18next.
13-32: Consider adding accessibility attributesThe component could benefit from improved accessibility by adding ARIA attributes to convey the urgency of the review deadline to screen readers.
Consider adding aria-label to better describe the urgency state:
{!reviewInfo ? ( - <p className="text-red-500 font-medium text-sm">ASAP</p> + <p className="text-red-500 font-medium text-sm" aria-label="Review needed as soon as possible">ASAP</p> ) : ( <div className={`text-sm font-medium ${reviewInfo.isUrgent ? "text-red-500" : ""}`} + aria-label={`Next review in ${reviewInfo.daysUntil} days on ${format(reviewInfo.nextReviewDate, "MMMM do, yyyy")}${reviewInfo.isUrgent ? ", urgent attention required" : ""}`} > {reviewInfo.daysUntil} days ( {format(reviewInfo.nextReviewDate, "MM/dd/yyyy")}) </div> )}apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/types.ts (1)
24-31: Good refactor to explicit props patternThe updated interface properly reflects the new data structure where evidence is now associated with members rather than users directly. This change follows good React patterns by making the component more declarative and pushing data fetching concerns up the component tree.
Consider adding JSDoc comments to better document the props interface and its usage.
+/** + * Props for the EvidenceDetails component. + * @property {Array<Member & {user: User}>} assignees - Available members that can be assigned to evidence. + * @property {Evidence & {assignee: Member & {user: User}}} evidence - The evidence item with its current assignee. + */ export interface EvidenceDetailsProps { assignees: (Member & { user: User; })[]; evidence: Evidence & { assignee: Member & { user: User; }; }; }apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/page.tsx (3)
15-16: Recommend adding error or loading state handling.
getAssignees()andgetEvidence()can each fail or return an empty result. Consider providing a fallback UI or error boundary to handle these scenarios gracefully, especially in a production environment.export default async function EvidencePage({ params }: EvidencePageProps) { const { id } = await params; + try { const assignees = await getAssignees(); const evidence = await getEvidence(id); return <EvidenceDetails assignees={assignees} evidence={evidence} />; + } catch (err) { + console.error(err); + return <div>Error loading evidence details. Please try again later.</div>; + } }Also applies to: 18-18
21-45: Validate session and handle potential DB errors.
getAssigneescurrently returns an empty array whenorgIdis unavailable. Depending on your application needs, you may want to:
- Throw an error if the session or
orgIdis invalid.- Provide additional logging for visibility.
- Consider whether to handle database query errors here (e.g., via try/catch).
const getAssignees = async () => { const session = await auth.api.getSession({ headers: await headers(), }); const orgId = session?.session.activeOrganizationId; if (!orgId) { - return []; + throw new Error("No active organization found in session."); } try { const assignees = await db.member.findMany({ where: { organizationId: orgId, role: { notIn: ["employee"], }, }, include: { user: true, }, }); return assignees; } catch (error) { + console.error("Failed to retrieve assignees:", error); + throw error; } };
47-62: Return a user-friendly error or a 404.Throwing a raw
Error("Evidence not found")is functional but might not be user-friendly in a production environment. Consider returning a 404 response or a custom exception that you can catch and render a more descriptive error page.const getEvidence = async (id: string) => { const evidence = await db.evidence.findUnique({ where: { id, }, include: { assignee: true, }, }); if (!evidence) { - throw new Error("Evidence not found"); + // Option 1: Throw a custom error type to be handled by a global error boundary + // or server code that can return a 404 status page + const error = new Error("Evidence not found"); + (error as any).statusCode = 404; + throw error; } return evidence; };apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/components/table/EvidenceListColumns.tsx (1)
32-33: Simplify the fallback color handlingThe use of
?? " "(two spaces) as a fallback value for the background color doesn't seem to have any meaningful effect. Consider using a proper fallback color or removing the fallback entirely.-backgroundColor: - EVIDENCE_STATUS_HEX_COLORS[ - row.original.status ?? "draft" - ] ?? " ", +backgroundColor: + EVIDENCE_STATUS_HEX_COLORS[ + row.original.status ?? "draft" + ],And similarly on lines 60-61:
-backgroundColor: - EVIDENCE_STATUS_HEX_COLORS[status ?? "draft"] ?? " ", +backgroundColor: + EVIDENCE_STATUS_HEX_COLORS[status ?? "draft"],Also applies to: 60-61
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/page.tsx (1)
128-140: Update comments to reflect the new status-based logicThe comments at lines 128-132 are now outdated as they refer to the old boolean-based status determination, while the code now uses enum values. Please update these comments to reflect the new approach.
-// status = published if published is true -// status = draft if published is false and isNotRelevant is false -// status = isNotRelevant if published is false and isNotRelevant is true -// status = needsReview if published is true and needs review +// We now use explicit status values: "published", "draft", "not_relevant" +// needsReview is determined separately based on review datesapps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/actions/updateEvidenceDetails.ts (1)
54-63: Reconsider when to update lastPublishedAtThe current implementation sets
lastPublishedAtto the current date for all updates, regardless of the status change. This might not be appropriate for all status transitions, such as when changing to "draft" or "not_relevant".Consider conditionally updating
lastPublishedAtonly when the status is set to "published":const payload: Pick< Evidence, "department" | "frequency" | "assigneeId" | "status" | "lastPublishedAt" > = { department, frequency, assigneeId, - lastPublishedAt: new Date(), + ...(status === "published" ? { lastPublishedAt: new Date() } : {}), status, };apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceFrequencySection.tsx (1)
29-33: Optionally consider additional frequency choices.The list includes Monthly, Quarterly, and Yearly. If there's a need for more granular intervals (e.g., Weekly), you can expand this array to meet evolving requirements.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDetails.tsx (2)
12-17: Provide a way to “undo” or clarify thenot relevantstate.Marking evidence as not relevant hides it from compliance, but consider adding context or an “undo” flow in case it’s marked this way accidentally.
20-20: Leverage translational or dynamic labeling of AlertTitle.Using
evidence.namedirectly is fine, but if dynamic renaming or translations are expected in the future, ensure that this is locally or centrally translated to preserve i18n consistency.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceStatusSection.tsx (1)
13-17: Consider dynamic generation of status options.While this hardcoded array works, consider dynamically generating status options from a shared enum or config object to ensure consistency and reduce duplication across components.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/actions/getOrganizationEvidenceTasks.ts (1)
71-73: Consider grouping similar status filters.The separate spread operators for
"published","draft", and"not_relevant"can be consolidated to reduce code duplication if future statuses are added. For instance, building an object with conditional keys might simplify the approach.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/ReviewSection.tsx (2)
64-73: Robust error-handling strategy.Using
onSuccessandonErrorcallbacks to manage toast messages keeps the user informed. Consider logging the error details more explicitly to help with production debugging (if that’s not already handled upstream).
137-145: Recommendations for button password or additional confirmation.Right now, hitting "Save" applies changes immediately. Consider adding a confirmation flow (e.g., a dialog) if these changes can have a high impact, or are irreversible. Might be overkill, but worth discussing with stakeholders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/app/src/actions/organization/lib/utils.ts(9 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/constants/evidence-status.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/page.tsx(4 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/actions/updateEvidenceDetails.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/AssigneeSection.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/DepartmentSection.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceAssigneeSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDepartmentSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDetails.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceFrequencySection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceNextReviewSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceStatusSection.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/FrequencySection.tsx(0 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/ReviewSection.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/types.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/actions/getOrganizationEvidenceTasks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/components/table/EvidenceListColumns.tsx(4 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/hooks/useEvidenceTableContext.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/types.ts(1 hunks)packages/db/prisma/migrations/20250403150029_update_evidence_statuses/migration.sql(1 hunks)packages/db/prisma/migrations/20250403150143_update_enum/migration.sql(1 hunks)packages/db/prisma/migrations/20250403152616_assignee_should_be_member_not_user/migration.sql(1 hunks)packages/db/prisma/migrations/20250403153215_idk/migration.sql(1 hunks)packages/db/prisma/schema/auth.prisma(1 hunks)packages/db/prisma/schema/evidence.prisma(1 hunks)
💤 Files with no reviewable changes (3)
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/FrequencySection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/AssigneeSection.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/DepartmentSection.tsx
🧰 Additional context used
🧬 Code Definitions (6)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/components/table/EvidenceListColumns.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/constants/evidence-status.ts (1)
EVIDENCE_STATUS_HEX_COLORS(23-27)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/page.tsx (1)
apps/app/src/locales/features/evidence.ts (1)
evidence(1-35)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/page.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDetails.tsx (1)
EvidenceDetails(9-36)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceStatusSection.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/constants/evidence-status.ts (1)
EVIDENCE_STATUS_HEX_COLORS(23-27)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/ReviewSection.tsx (6)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/actions/updateEvidenceDetails.ts (1)
updateEvidenceDetails(18-84)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceAssigneeSection.tsx (1)
EvidenceAssigneeSection(24-134)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceStatusSection.tsx (1)
EvidenceStatusSection(19-74)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDepartmentSection.tsx (1)
EvidenceDepartmentSection(19-60)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceFrequencySection.tsx (1)
EvidenceFrequencySection(19-60)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceNextReviewSection.tsx (1)
EvidenceNextReviewSection(4-33)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/constants/evidence-status.ts (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/data/getEvidenceDashboard.ts (1)
EvidenceStatus(5-5)
🔇 Additional comments (46)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/hooks/useEvidenceTableContext.tsx (1)
196-198: LGTM: Correctly updated assignee data accessThe changes properly reflect the new data model structure where assignee is now a Member with a relationship to a User, rather than being a User directly. Accessing the name and image through
task.assignee.useraligns with the database schema changes.packages/db/prisma/migrations/20250403152616_assignee_should_be_member_not_user/migration.sql (1)
1-5: LGTM: Well-structured migration with appropriate constraintsThis migration correctly updates the foreign key relationship for Evidence from User to Member. The constraint includes appropriate ON DELETE SET NULL and ON UPDATE CASCADE behaviors to maintain referential integrity. The migration name is also clear and descriptive of its purpose.
packages/db/prisma/schema/auth.prisma (1)
90-90: Solid improvement to the data modelMoving the evidence relationship from the
Usermodel to theMembermodel is architecturally sound. This change properly contextualizes evidence within organizations, allowing the same user to have different evidence assignments across different organizations.This change aligns well with the modified
Evidencemodel that now referencesMemberinstead ofUserfor its assignee relationship.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/types.ts (1)
2-2: LGTM: Required imports addedThe import statement correctly includes all the necessary types from the DB package.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/types.ts (1)
4-11: Type structure aligns with data model changesThe updated
assigneeproperty structure correctly reflects the new relationship where assignees are nowMemberobjects with a nesteduserproperty. The type properly handles nullability at both the assignee level and for the user properties.This change ensures type safety when accessing user information through the updated path (
assignee.user.nameinstead ofassignee.name).packages/db/prisma/schema/evidence.prisma (4)
10-10: Smart conversion from boolean flags to enumConverting the previous boolean flags (
published,isNotRelevant) to a singlestatusenum is a great improvement. This makes the states mutually exclusive, easier to extend in the future, and provides more semantic clarity.The default value of
draftis also sensible as the initial state for new evidence.
19-19: Relationship update matches schema changesChanging the
assigneerelation to referenceMemberinstead ofUseraligns with the earlier changes inauth.prismaand makes the data model more consistent.
27-30: Well-defined enum valuesThe
EvidenceStatusenum has clear, descriptive values that correspond to the previous boolean flags while providing a more structured approach. This will make the code more maintainable and the UI logic simpler.
25-25:Details
❓ Verification inconclusive
Verify impact of removed index on assigneeId
The index on
assigneeIdhas been removed. While this simplifies the schema, it might impact query performance if there are frequent queries filtering onassigneeId.
🏁 Script executed:
#!/bin/bash # Check if there are queries in the codebase that filter by assigneeId echo "Searching for queries that might be affected by the removal of assigneeId index..." # Look for prisma queries that might use assigneeId rg -A 3 -B 3 "where.*assigneeId" --type=ts # Look for places where evidence is queried by assignee rg -A 3 -B 3 "evidence.*assignee" --type=ts | grep -i "find\|where\|filter"Length of output: 687
Action Required: Assess Performance Impact of Removed
assigneeIdIndexThe removed index on
assigneeIdwarrants a closer look since we found multiple queries filtering on this field. In particular, our codebase search revealed that:
- In
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/tests/all/[testId]/actions/assignTest.ts, a query uses awhereclause that could be impacted.- In
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/actions/assignEvidence.ts, a similar query construction is present.- In
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/data/getEvidenceDashboard.ts, filtering onassigneeId(via a negation in afilterfunction) is observed.Please verify that the performance of these queries remains acceptable without an index on
assigneeId. If these queries are executed frequently and performance degrades, consider either reintroducing the index or optimizing these queries.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/page.tsx (2)
1-4: Imports look good.All imported modules appear relevant and correctly referenced. No issues detected.
7-9:Details
❓ Verification inconclusive
Consider verifying the usage of a Promise for
params.It’s somewhat unusual to define
paramsasPromise<{ id: string }>in a Next.js route. Typically,paramsis passed directly as an object. Please confirm that the calling code or Next.js configuration indeed providesparamsas a promise to avoid unexpected runtime issues.Run the following script to search for the parameter usage in the codebase and confirm it returns a promise rather than a plain object:
Also applies to: 13-13
🏁 Script executed:
#!/bin/bash # Description: Look for references to `EvidencePage` usage and check how params is passed. rg -A 5 'EvidencePage\({\s*params:'Length of output: 160
#!/bin/bash # Revised search for EvidencePage usage with a corrected regex pattern, # which checks if `params` is being passed as a Promise. rg -P -A 5 'EvidencePage\(\{\s*params:'Double-check Promise Usage for Route Params
The route file declares
paramsasPromise<{ id: string }>(lines 7–9). This deviates from the typical Next.js pattern where route parameters are directly provided as an object. Please verify that this asynchronous signature is intentional—that the calling code or Next.js configuration indeed suppliesparamsas a promise.
- Confirm with the revised regex search (above) that any usage of
EvidencePagealigns with an expectation forparamsas a promise.- If no evidence supports the promise-based pattern, consider updating
paramsto a plain object to avoid potential runtime issues.apps/app/src/actions/organization/lib/utils.ts (8)
17-20: No functional changes ingetPolicyById.Only formatting/indentation updates. Looks cleaner and more consistent.
28-31: No functional changes ingetEvidenceById.The indentation adjustments align well with standard code style. No issues detected.
45-52: No functional changes ingetRelevantControls.The re-indentation appears correct and improves readability.
69-161: No functional changes increateFrameworkInstance.These updates strictly pertain to indentation and code style, with no logic changes. Everything looks in order.
179-238: No functional changes increateRequirementMaps.Only spacing and indentation were updated, no issues with functionality.
257-326: No functional changes increateOrganizationPolicies.Indentation updates. Code remains logically sound.
345-422: No functional changes increateOrganizationEvidence.Code style changes enhance consistency. Approved.
442-555: No functional changes increateControlArtifacts.The formatting updates do not affect the underlying logic.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/list/components/table/EvidenceListColumns.tsx (2)
63-67: Good implementation of status formattingThe status formatting logic is well-implemented, splitting by underscore, capitalizing each word, and joining with spaces to create a human-readable format.
172-177: Updated access pattern for assignee user dataThe change to access user information through
assignee.userinstead of directly fromassigneealigns well with what appears to be a database schema change. This update ensures the component correctly displays user information with the new data structure.Also applies to: 179-179
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/page.tsx (2)
63-63: Consistent usage of enum status valuesGood job transitioning from boolean flags to descriptive status enum values for the evidence queries. This improves code readability and consistency.
Also applies to: 69-69, 75-75
91-100: Updated schema structure for assignee dataThe changes correctly adapt to the updated schema structure where user data is nested within the assignee object. This ensures the application properly accesses and utilizes user information.
packages/db/prisma/migrations/20250403150143_update_enum/migration.sql (1)
1-16: Well-structured enum migrationThis migration is well-structured and safely updates the
EvidenceStatusenum to use the new values. It follows best practices by:
- Creating a new enum type with the updated values
- Updating the column to use the new type
- Properly handling the transition with a transaction
- Setting an appropriate default value
Make sure there are no existing records with the value 'notRelevant' before running this migration, as mentioned in the warning.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/actions/updateEvidenceDetails.ts (2)
10-16: Good schema validation with ZodThe Zod schema properly validates all input parameters, ensuring type safety and data consistency when updating evidence details.
18-84: Well-implemented server actionThe server action follows best practices with proper authentication, error handling, and data validation. It verifies that the user has access to the organization and that the evidence exists before attempting to update it.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceFrequencySection.tsx (2)
24-27: Validate type safety forFrequencyconversion.Casting
valuetoFrequencycould mask invalid strings which are not part of theFrequencytype. Consider a stricter runtime assert, or ensure all possible string literals are accounted for.Would you like a script that scans for usage of
EvidenceFrequencySectionto confirm only valid frequency values are being passed from the parent?
35-60: Neat and fluid user interface structure.The layout and styling create an intuitive frequency selection flow. The usage of
SelectTrigger,SelectValue, andSelectContentaligns well with the desired UX pattern.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDetails.tsx (2)
4-4: Straightforward import and signature changes.Imports for
FileIconandReviewSectionare correct, and switching props to acceptassignees/evidencesimplifies the logic by removing internal data fetching.Also applies to: 7-7, 9-9
31-31: Prop assignment looks aligned with the new design.Passing
assignees={assignees}to theReviewSectionharmonizes well with the newly modularized approach to evidence management.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceAssigneeSection.tsx (2)
53-64: Ensure correct handling of empty or absolute avatar image URLs.
getImageUrlforwards relative URLs as-is, which is acceptable if the server correctly serves them. If there’s a scenario where images need an absolute domain prefix, ensure that’s handled in an environment-aware manner.
104-128: Clear and concise visualization of assignee items.Displaying avatars with fallback initials enhances the user experience, and the minimal
SelectItemstyling works well for clarity.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceStatusSection.tsx (1)
28-74: Component design looks solid; ensure wide test coverage.This new
EvidenceStatusSectionclearly differentiates the status selection. The approach to color-coding statuses viaEVIDENCE_STATUS_HEX_COLORSis a neat UI enhancement. Ensure that edge cases (e.g., unknown statuses, or statuses that conflict with upstream logic) are tested to prevent data mismatches.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/EvidenceDepartmentSection.tsx (2)
29-31: Validate the decision to hide "none" from the department options.Filtering out "none" might be intentional for user flow, but confirm if there's a scenario where "none" is a valid input. If so, consider providing a user-facing way to indicate no department selection.
19-60: Code style and logic are straightforward.This new
EvidenceDepartmentSectionproperly handles department selection, including a fallback to disable the dropdown. The usage ofSelectfrom@bubba/ui/selectis consistent with the rest of the codebase. The approach is neat and modular.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/actions/getOrganizationEvidenceTasks.ts (4)
29-33: Check if adding new statuses requires broader logic changes.The schema now supports
"not_relevant"as a status. Verify any references in the codebase (e.g., other query filters, dashboards) to ensure consistency and correctness when introducing this new status.
81-82: Potential overlap with status filters and relevance.Lines 81-82 overwrite the earlier status filters if
relevanceis also provided, forcing status to"published"or"not_relevant". Ensure this is intended behavior, as it may ignore the explicitstatusparameter whenrelevanceis set.
105-107: Efficient counting approach is fine here.Using
count()with the samewhereclause is straightforward and efficient enough for typical pagination. For larger datasets, you might consider caching or advanced indexing strategies, but this looks good as is.
114-135: Sensible inclusion of assignee user details in the query.Including the nested user fields helps reduce additional queries on the client side. This is a clean approach that fosters a single retrieval pass for each evidence item.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/[id]/components/ReviewSection.tsx (3)
52-61: Well-managed component state.Storing the relevant fields (
frequency,department,assigneeId,status) in local state centralizes data management. It also ensures a clear workflow for saving changes. This is a good pattern for well-structured UI components.
75-84: Potential race condition check.
setIsSaving(true)is called immediately before invokingupdateDetailsAction. If the user double-clicks quickly, React can batch the updates, but typically the button is disabled byisSavingso it’s likely safe. Just confirm no concurrency issues occur if the save button is clicked multiple times.
128-134: Useful separate UI forEvidenceFrequencySection.Breaking out the frequency into its own modular section makes the code more maintainable and fosters reusability. Keep up the same pattern for other UI pieces.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/(overview)/constants/evidence-status.ts (4)
5-6: Good addition of type import.Importing the
EvidenceStatustype from a centralized location promotes type consistency across the codebase, which is good practice.
8-13: Improved type safety and naming.The renaming from
STATUS_PRIORITYtoEVIDENCE_STATUS_PRIORITYand explicit typing asEvidenceStatus[]enhances clarity and type safety. The changes align with the new evidence status enum defined in the database schema.
23-27: Consistent hex color mapping.The hex color mapping is appropriate with:
- draft: amber/orange (#f59e0b)
- not_relevant: red (#ef4444)
- published: green (#10b981)
This color scheme properly conveys the semantic meaning of each status.
30-37: Well-structured translation keys.The translation keys follow a consistent pattern and include all the necessary statuses. This structure will support proper internationalization of the evidence status labels.
| /* | ||
| Warnings: | ||
|
|
||
| - You are about to drop the column `isNotRelevant` on the `Evidence` table. All the data in the column will be lost. | ||
| - You are about to drop the column `published` on the `Evidence` table. All the data in the column will be lost. | ||
|
|
||
| */ | ||
| -- CreateEnum | ||
| CREATE TYPE "EvidenceStatus" AS ENUM ('draft', 'published', 'notRelevant'); | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "Evidence" DROP COLUMN "isNotRelevant", | ||
| DROP COLUMN "published", | ||
| ADD COLUMN "status" "EvidenceStatus" DEFAULT 'draft'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify data migration strategy to prevent data loss.
Dropping isNotRelevant and published columns without mapping existing data to the new status enum can cause irreversible data loss. Ensure you have a migration or fallback plan to handle existing records if these columns contain important information.
🏁 Script executed:
#!/bin/bash
# Description: Inspect existing usage of `isNotRelevant` and `published` fields before dropping them.
# 1) Search for references in the code that might rely on these columns
rg 'isNotRelevant|published' -A 5
# 2) (Optional) Check the count of data in these columns if any code references are found
# This step may require direct DB access or an environment with DB credentials,
# which we do not perform in this sandbox environment.Length of output: 67221
Action Required: Implement Data Mapping for Evidence Status Migration
The current migration (packages/db/prisma/migrations/20250403150029/migration.sql) drops the isNotRelevant and published columns and creates the new status enum—defaulting to 'draft'—without preserving any existing data. Our codebase (e.g., in evidence overview pages, filtering logic, and status toggling actions) relies heavily on the dropped fields to determine an evidence record’s status. This means that records previously marked as published or not relevant will be reset to draft, potentially causing unintended behavior.
Recommended next steps:
- Update the migration to include a data transformation step that maps:
published: true→'published'published: falseandisNotRelevant: true→'not_relevant'(ensure consistency with enum naming)- All others →
'draft'
- Verify changes across the codebase to replace any references to the old columns with the new
statusfield and adjust enum values (note the mix of"notRelevant"vs."not_relevant").
| <div className="md:hidden"> | ||
| <StatusPolicies | ||
| status={row.original.published ? "published" : "draft"} | ||
| /> | ||
| <div className="hidden md:flex gap-2"> | ||
| <div className={cn("flex items-center gap-2")}> | ||
| <div | ||
| className={cn("size-2.5")} | ||
| style={{ | ||
| backgroundColor: | ||
| EVIDENCE_STATUS_HEX_COLORS[ | ||
| row.original.status ?? "draft" | ||
| ] ?? " ", | ||
| }} | ||
| /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Fix the visibility classes in the mobile view
There's an issue with the visibility classes. You have a hidden md:flex div inside a md:hidden div, which means the content would never be visible since the parent hides on medium screens while the child only shows on medium screens.
-<div className="md:hidden">
- <div className="hidden md:flex gap-2">
+<div className="md:hidden">
+ <div className="flex gap-2">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="md:hidden"> | |
| <StatusPolicies | |
| status={row.original.published ? "published" : "draft"} | |
| /> | |
| <div className="hidden md:flex gap-2"> | |
| <div className={cn("flex items-center gap-2")}> | |
| <div | |
| className={cn("size-2.5")} | |
| style={{ | |
| backgroundColor: | |
| EVIDENCE_STATUS_HEX_COLORS[ | |
| row.original.status ?? "draft" | |
| ] ?? " ", | |
| }} | |
| /> | |
| </div> | |
| </div> | |
| <div className="md:hidden"> | |
| <div className="flex gap-2"> | |
| <div className={cn("flex items-center gap-2")}> | |
| <div | |
| className={cn("size-2.5")} | |
| style={{ | |
| backgroundColor: | |
| EVIDENCE_STATUS_HEX_COLORS[ | |
| row.original.status ?? "draft" | |
| ] ?? " ", | |
| }} | |
| /> | |
| </div> | |
| </div> |
| const [selectedAssignee, setSelectedAssignee] = useState< | ||
| (Member & { user: User }) | null | ||
| >(null); | ||
|
|
||
| const handleAssigneeChange = (value: string) => { | ||
| const newAssigneeId = value === "none" ? null : value; | ||
| onAssigneeChange(newAssigneeId); | ||
|
|
||
| if (newAssigneeId && assignees) { | ||
| const assignee = assignees.find((a) => a.id === newAssigneeId); | ||
| if (assignee) { | ||
| setSelectedAssignee({ | ||
| ...assignee, | ||
| user: assignee.user, | ||
| }); | ||
| } else { | ||
| setSelectedAssignee(null); | ||
| } | ||
| } else { | ||
| setSelectedAssignee(null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep selectedAssignee in sync with prop changes.
Currently, the state is updated only inside handleAssigneeChange. If assigneeId changes from the parent without user interaction, the UI might not reflect the update. A small useEffect could help maintain consistency.
+import { useEffect, useState } from "react";
...
useEffect(() => {
if (assigneeId && assignees) {
const found = assignees.find((a) => a.id === assigneeId);
setSelectedAssignee(found || null);
} else {
setSelectedAssignee(null);
}
}, [assigneeId, assignees]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [selectedAssignee, setSelectedAssignee] = useState< | |
| (Member & { user: User }) | null | |
| >(null); | |
| const handleAssigneeChange = (value: string) => { | |
| const newAssigneeId = value === "none" ? null : value; | |
| onAssigneeChange(newAssigneeId); | |
| if (newAssigneeId && assignees) { | |
| const assignee = assignees.find((a) => a.id === newAssigneeId); | |
| if (assignee) { | |
| setSelectedAssignee({ | |
| ...assignee, | |
| user: assignee.user, | |
| }); | |
| } else { | |
| setSelectedAssignee(null); | |
| } | |
| } else { | |
| setSelectedAssignee(null); | |
| } | |
| import { useEffect, useState } from "react"; | |
| const [selectedAssignee, setSelectedAssignee] = useState< | |
| (Member & { user: User }) | null | |
| >(null); | |
| const handleAssigneeChange = (value: string) => { | |
| const newAssigneeId = value === "none" ? null : value; | |
| onAssigneeChange(newAssigneeId); | |
| if (newAssigneeId && assignees) { | |
| const assignee = assignees.find((a) => a.id === newAssigneeId); | |
| if (assignee) { | |
| setSelectedAssignee({ | |
| ...assignee, | |
| user: assignee.user, | |
| }); | |
| } else { | |
| setSelectedAssignee(null); | |
| } | |
| } else { | |
| setSelectedAssignee(null); | |
| } | |
| }; | |
| useEffect(() => { | |
| if (assigneeId && assignees) { | |
| const found = assignees.find((a) => a.id === assigneeId); | |
| setSelectedAssignee(found || null); | |
| } else { | |
| setSelectedAssignee(null); | |
| } | |
| }, [assigneeId, assignees]); |
| export const EVIDENCE_STATUS_COLORS = { | ||
| draft: "bg-[var(--chart-open)]", | ||
| needsReview: "bg-[hsl(var(--destructive))]", | ||
| empty: "bg-[var(--chart-pending)]", | ||
| not_relevant: "bg-[hsl(var(--destructive))]", | ||
| published: "bg-[hsl(var(--destructive))]", | ||
| } as const; |
There was a problem hiding this comment.
Color inconsistency for "published" status.
There appears to be a inconsistency in the color scheme. The "published" status uses a destructive (likely red) color class, but this seems inappropriate for what appears to be a positive status. In the hex color definitions below, "published" correctly uses a green color.
export const EVIDENCE_STATUS_COLORS = {
draft: "bg-[var(--chart-open)]",
not_relevant: "bg-[hsl(var(--destructive))]",
- published: "bg-[hsl(var(--destructive))]",
+ published: "bg-[hsl(var(--success))]",
} as const;📝 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.
| export const EVIDENCE_STATUS_COLORS = { | |
| draft: "bg-[var(--chart-open)]", | |
| needsReview: "bg-[hsl(var(--destructive))]", | |
| empty: "bg-[var(--chart-pending)]", | |
| not_relevant: "bg-[hsl(var(--destructive))]", | |
| published: "bg-[hsl(var(--destructive))]", | |
| } as const; | |
| export const EVIDENCE_STATUS_COLORS = { | |
| draft: "bg-[var(--chart-open)]", | |
| not_relevant: "bg-[hsl(var(--destructive))]", | |
| published: "bg-[hsl(var(--success))]", | |
| } as const; |
This branch had an error being deployed
1 participant
Summary by CodeRabbit