Overhaul worker records and profile sections - #20
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe worker record overhaul moves identity data into worker documents. It adds a five-stage create/edit form, shared file handling, section-based worker details, attendance data, import validation, audit redaction, updated reports, database functions, migrations, and responsive end-to-end coverage. ChangesWorker record model and persistence
Worker record form and file workflow
Worker detail sections and attendance
Imports, audit, and reports
Validation and test coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
actor CEO
participant WorkerRecordForm
participant WorkerActions
participant Database
participant Storage
CEO->>WorkerRecordForm: Complete five worker-record stages
WorkerRecordForm->>WorkerActions: Submit metadata, documents, and optional photo
WorkerActions->>Database: Save worker and document metadata
Database-->>WorkerActions: Return worker ID and document mapping
WorkerActions->>Storage: Upload validated document or photo
Storage-->>WorkerActions: Return upload result
WorkerActions-->>WorkerRecordForm: Return success or partial-upload failures
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/workers/[workerId]/documents/[documentId]/route.ts (1)
15-23: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit active-user/role check before serving worker documents.
auth()only proves the Clerk session exists; it does not loadapplication_users.is_activeand role. UserequireRole("CEO")orrequireRole("FOREMAN")before queryingworker_documents, then handle inactive/unmapped access without leaking document buckets/paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/workers/`[workerId]/documents/[documentId]/route.ts around lines 15 - 23, Update the route handler around auth() to call requireRole("CEO") or requireRole("FOREMAN") before any worker_documents query, and reject inactive or unmapped users with the existing unauthorized/not-found response behavior. Ensure authorization occurs before document lookup or any bucket/path data can be exposed, while preserving the current UUID validation flow.Source: Path instructions
🟡 Minor comments (18)
src/app/ceo/settings/page.tsx-485-492 (1)
485-492: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the violet accent for the new checkbox controls.
The new inputs use
accent-stone-950. Replace it with the repository’s approved violet accent token so these controls match the settings UI.As per coding guidelines, use a consistent violet accent in TSX UI.
Also applies to: 509-516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/settings/page.tsx` around lines 485 - 492, Update the new checkbox inputs in the settings UI, including the controls near “Collect document number” and the additional checkbox, to replace accent-stone-950 with the repository-approved violet accent token. Keep the existing checkbox styling and behavior unchanged.Source: Coding guidelines
src/components/phase3/worker-record-form/stages.tsx-142-177 (1)
142-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLink the select error text to the control.
Both selects set
aria-invalidbut omitaria-describedby. TheErrorTextelements render withid="tradeId-error"andid="skillLevelId-error", so a screen reader does not announce the reason. The text inputs in this file already usearia-describedby.♿ Proposed fix
aria-invalid={Boolean(errors.tradeId)} + aria-describedby={errors.tradeId ? "tradeId-error" : undefined}aria-invalid={Boolean(errors.skillLevelId)} + aria-describedby={ + errors.skillLevelId ? "skillLevelId-error" : undefined + }As per coding guidelines: "Preserve keyboard access, visible labels, logical focus order, accessible status text".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/stages.tsx` around lines 142 - 177, Update the tradeId and skillLevelId select elements in the worker record form to include aria-describedby values referencing their corresponding ErrorText IDs: tradeId-error and skillLevelId-error. Keep the existing validation, labels, and select behavior unchanged.Source: Coding guidelines
supabase/migrations/20260803090000_worker_record_overhaul.sql-251-269 (1)
251-269: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTurning off
is_repeatablecan fail with an opaque unique-index error.This trigger sets
type_repeatable = falseon every document of the type. If a worker holds more than oneACTIVEdocument of that type, the partial unique indexworker_one_active_nonrepeatable_documentrejects the update. The CEO then sees a raw database error on the document-type settings page.Raise an explicit exception with a clear message when a conflict exists, so the settings page can report the reason.
♻️ Proposed guard
begin if new.is_repeatable is distinct from old.is_repeatable then + if not new.is_repeatable and exists ( + select 1 + from public.worker_documents + where document_type_id = new.id + and file_kind = 'DOCUMENT' + and status = 'ACTIVE' + group by worker_id + having count(*) > 1 + ) then + raise exception + 'Some workers hold more than one active % document', new.name; + end if; update public.worker_documents🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260803090000_worker_record_overhaul.sql` around lines 251 - 269, Update private.sync_documents_after_type_change to detect, before propagating is_repeatable = false, whether any worker has multiple ACTIVE documents for the affected document type that would violate worker_one_active_nonrepeatable_document; raise an explicit exception with a clear user-facing message when such a conflict exists, otherwise preserve the existing update and return behavior.e2e/phase3.spec.ts-163-165 (1)
163-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSign out before switching to FOREMAN.
This test signs in as FOREMAN while the CEO session is still active. Clerk’s role-switch flow signs out the current user so the new ticket applies cleanly. Add
await clerk.signOut({ page });before thesignIn(page, "FOREMAN")flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/phase3.spec.ts` around lines 163 - 165, In the test flow before signIn(page, "FOREMAN"), call clerk.signOut({ page }) to clear the active CEO session, then retain the existing FOREMAN sign-in and navigation steps unchanged.src/components/phase3/worker-record-form/review-summary.tsx-172-209 (1)
172-209: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFormat the money values before display.
values.hourlyRateandvalues.foodDeductionare raw input strings. Input such as15.5or15renders as "RM 15.5" or "RM 15", and an empty stored value renders as "RM 0.00" only through the||fallback. Convert to sen and format with a fixed two-decimal formatter so Review shows the exact amount that is saved.As per coding guidelines: "Store money as integer sen ... use
Asia/Kuala_Lumpurfor business dates".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/review-summary.tsx` around lines 172 - 209, Update the money display in the review summary’s hourly-rate and food-deduction ValueRow entries to convert the raw input strings to integer sen and format them with the existing fixed two-decimal money formatter before rendering. Preserve the current zero fallback and food-deduction “/ month” suffix, and apply the same formatting to edit-mode before values so both displayed amounts match the saved precision.Source: Coding guidelines
src/components/phase3/worker-record-form/stepper.tsx-42-55 (1)
42-55: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid the duplicate stage label for screen readers, and raise the description text size.
From the
smbreakpoint, the visible label at line 46 and thesr-onlylabel at line 53 both containitem.label, so assistive technology announces the stage name twice. Mark the visible blockaria-hiddenand keep thesr-onlytext as the single accessible name. Also,text-[10px]at line 48 is small for supporting text;text-xsis more legible.♻️ Proposed change
- <span className="ml-3 hidden min-w-0 sm:block"> + <span className="ml-3 hidden min-w-0 sm:block" aria-hidden="true"> <span className={`block text-xs font-semibold ${active ? "text-slate-950" : "text-slate-500"}`} > {item.label} </span> - <span className="mt-1 block truncate text-[10px] text-slate-500"> + <span className="mt-1 block truncate text-xs text-slate-500"> {item.description} </span> </span>As per coding guidelines: "Preserve keyboard access, visible labels, logical focus order, accessible status text".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/stepper.tsx` around lines 42 - 55, Update the visible label/description wrapper around item.label and item.description to use aria-hidden so screen readers rely solely on the existing sr-only status text, while preserving keyboard access and focus order. Change the description styling from text-[10px] to text-xs.Source: Coding guidelines
src/components/phase3/worker-record-form/review-summary.tsx-39-41 (1)
39-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIncrease the
Editcontrol target size for mobile.
size="sm"produces a control below the required mobile target area. TheEditbutton is the primary way to return to a stage from Review. Give it a minimum height of about 44 px.♻️ Proposed change
- <Button type="button" variant="ghost" size="sm" onClick={edit}> + <Button + type="button" + variant="ghost" + onClick={edit} + className="min-h-11" + > Edit </Button>As per coding guidelines: "mobile control target areas of approximately 44–52 px".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/review-summary.tsx` around lines 39 - 41, Update the Edit Button in the review summary to provide a minimum height of approximately 44 px for mobile control targets, while preserving its existing type, ghost variant, small sizing, and edit handler.Source: Coding guidelines
src/components/phase3/worker-record-form/form.tsx-246-252 (1)
246-252: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAnnounce server errors assertively, and collapse the duplicate class branch.
This container always uses
role="status"witharia-live="polite", including whenstate.status === "error". A screen reader then defers the failure message, and the user may continue editing. Switch the role and live region based on the status. Also, thestate.status === "error"branch and thestate.partialUploadFailures?.lengthbranch at line 250 produce identical classes.♻️ Proposed change
+ {state.message ? ( + (() => { + const isProblem = + state.status === "error" || + Boolean(state.partialUploadFailures?.length); + return ( <div - role="status" - aria-live="polite" - className={`mt-4 rounded-lg border p-4 text-sm ${state.status === "error" ? "border-amber-200 bg-amber-50 text-amber-950" : state.partialUploadFailures?.length ? "border-amber-200 bg-amber-50 text-amber-950" : "border-emerald-200 bg-emerald-50 text-emerald-900"}`} + role={isProblem ? "alert" : "status"} + aria-live={isProblem ? "assertive" : "polite"} + className={`mt-4 rounded-lg border p-4 text-sm ${isProblem ? "border-amber-200 bg-amber-50 text-amber-950" : "border-emerald-200 bg-emerald-50 text-emerald-900"}`} >As per coding guidelines: "Preserve keyboard access, visible labels, logical focus order, accessible status text".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/form.tsx` around lines 246 - 252, Update the message container in the form component to use an assertive live region and alert semantics when state.status is "error", while preserving the polite status behavior for non-errors. Simplify the className conditional by combining the error and partialUploadFailures branches that produce the same amber classes, without changing the existing success styling.Source: Coding guidelines
src/components/phase3/worker-record-form/form.tsx-349-388 (1)
349-388: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the bottom padding breakpoint with the action bar, and check the control target size.
The action bar is
fixeduntil themdbreakpoint. The form padding drops frompb-28topb-20at thesmbreakpoint. Betweensmandmdthe bar remains fixed atbottom-[calc(4.25rem+env(safe-area-inset-bottom))], so the reserved 5 rem is smaller than the offset plus the bar height. The last form content can sit behind the bar in that range. Switch the padding atmd.Also confirm the
BackandContinuecontrol height. The defaultButtonsize in this project may be under the required mobile target area.♻️ Proposed change
- className="relative mt-5 pb-28 sm:pb-20" + className="relative mt-5 pb-28 md:pb-20"As per coding guidelines: "mobile control target areas of approximately 44–52 px" and "avoid horizontal page scrolling".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/form.tsx` around lines 349 - 388, Update the form’s responsive bottom padding so the reduced padding begins at the md breakpoint, matching the action bar’s fixed-to-sticky transition and preserving enough space for the bar above it. In the navigation controls rendered by the stage action area, verify the Back and Continue Button instances meet the project’s approximately 44–52px mobile target height, adjusting their sizing classes or props as needed without introducing horizontal overflow.Source: Coding guidelines
src/components/phase3/worker-detail/section-picker.tsx-31-50 (1)
31-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFixed
grid-cols-6does not match the variable section count. Both the rendered navigation and its loading skeleton hardcode six columns, while the CEO list has six entries and the foreman list has four. The columns and the skeleton placeholders then misalign with the actual sections.
src/components/phase3/worker-detail/section-picker.tsx#L31-L50: derive the column count fromsections.lengthinstead of the fixedgrid-cols-6class.src/components/operations/route-loading.tsx#L165-L179: derive the column count from the rendered label array so the foreman skeleton shows four aligned columns.As per coding guidelines: "Preserve keyboard access, visible labels, logical focus order, accessible status text, correctly aligned controls, and mobile control target areas of approximately 44–52 px."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/section-picker.tsx` around lines 31 - 50, The navigation in src/components/phase3/worker-detail/section-picker.tsx#L31-L50 and its skeleton in src/components/operations/route-loading.tsx#L165-L179 hardcode six grid columns despite variable section counts. Derive each grid’s column count from its corresponding rendered array length so CEO sections use six columns and foreman sections use four, while preserving existing labels, keyboard access, and focus order.Source: Coding guidelines
src/components/operations/route-loading.tsx-195-198 (1)
195-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHide the decorative icon from assistive technology.
ChevronLeftat Line 196 has noaria-hidden="true", while the same icon at Line 161 in this file has it. Keep the attribute consistent so the back label reads as one item.♿ Proposed fix
- <ChevronLeft className="size-4" /> + <ChevronLeft className="size-4" aria-hidden="true" />As per coding guidelines: "Preserve keyboard access, visible labels, logical focus order, accessible status text, correctly aligned controls, and mobile control target areas of approximately 44–52 px."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/operations/route-loading.tsx` around lines 195 - 198, Update the ChevronLeft component in the back-label span to include aria-hidden="true", matching the corresponding icon usage elsewhere in the component and keeping the visible backLabel as the sole accessible content.Source: Coding guidelines
src/components/phase3/worker-detail/section-picker.tsx-71-89 (1)
71-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the malformed
SheetClosecomposition.
@base-ui/react@1.6.0Dialog.Closedoes not accept a React element viarender; useasChildwithSheetClose/Linkor render the label/icon inside the actual rendered close button. As written, the label andCheckicon are props/children of a non-existentrenderprop and will not render inside the link.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/section-picker.tsx` around lines 71 - 89, Update the section picker composition around SheetClose so it uses the supported asChild pattern with Link, or renders the label and conditional Check inside the actual close button; remove the invalid render prop from SheetClose while preserving the link href, active aria-current state, styling, and selection indicator.src/app/foreman/workers/[workerId]/page.tsx-88-92 (1)
88-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe section picker uses a fixed six-column desktop grid.
WorkerSectionPickerrendersgrid-cols-6(src/components/phase3/worker-detail/section-picker.tsxLines 30-33). The foreman page supplies four sections, so two grid cells stay empty and the tabs do not fill the row. Derive the column count fromsections.length.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/foreman/workers/`[workerId]/page.tsx around lines 88 - 92, Update WorkerSectionPicker and its usage on the foreman worker page so the grid column count is derived from sections.length rather than fixed at six. Preserve the existing section rendering while ensuring the four supplied sections fill the row without empty grid cells.src/app/ceo/workers/new/page.tsx-42-48 (1)
42-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the case where no pinned document type exists.
pinnedTypesdrops any code that has no activedocument_typesrow. If all three codes are missing or inactive, the create form opens with zero document rows, and the empty-state guard at Line 81 does not cover this. Add a guard that directs the user to Settings when the required document types are absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/new/page.tsx` around lines 42 - 48, Update the create-form initialization around pinnedTypes to handle an empty result: when no required pinned document types are available, direct the user to Settings before rendering or opening the form. Preserve the existing behavior for available document types and ensure the guard covers the all-missing or inactive case.src/components/phase3/worker-detail/document-list.tsx-68-80 (1)
68-80: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not render document metadata values verbatim.
String(value)prints every metadata value. Document metadata can hold identity data such as a permit or licence number, and the coding guidelines forbid showing full identity-document numbers in the UI. The adjacentdocument_numberis masked, so the metadata line becomes the weaker path. Mask values for identifier-like keys, or render only an approved key allow-list.As per coding guidelines: "never reveal secrets, tokens, passwords, private files, or full identity-document numbers in UI, logs, screenshots, or test output."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/document-list.tsx` around lines 68 - 80, Update the metadata rendering in the document list to avoid displaying sensitive values verbatim: mask identifier-like keys, including permit or licence numbers, or restrict output to an approved metadata key allow-list. Preserve the existing filtering and formatting for safe metadata while ensuring full identity-document numbers cannot reach the String(value) display path.Source: Coding guidelines
src/lib/phase3/data.ts-413-425 (1)
413-425: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the primary identifier selection deterministic.
documentsis ordered bycreated_atdescending, sofindreturns whichever of CNIC or PASSPORT was created last. A worker that holds both documents can show a different primary identifier after any document edit. Select CNIC first, then PASSPORT.♻️ Proposed fix
- const primaryDocument = documents.find((document) => { - const code = document.document_type_id - ? documentTypes.get(document.document_type_id)?.system_code - : null; - return ( - document.file_kind === "DOCUMENT" && - Boolean(document.document_number) && - ["CNIC", "PASSPORT"].includes(code ?? "") - ); - }); + const documentByCode = (systemCode: string) => + documents.find( + (document) => + document.file_kind === "DOCUMENT" && + Boolean(document.document_number) && + document.document_type_id && + documentTypes.get(document.document_type_id)?.system_code === + systemCode, + ); + const primaryDocument = documentByCode("CNIC") ?? documentByCode("PASSPORT");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase3/data.ts` around lines 413 - 425, Update the primaryDocument selection to prioritize eligible CNIC documents before eligible PASSPORT documents, instead of relying on the documents array order. Preserve the existing DOCUMENT kind and document_number requirements, and keep primaryType derived from the selected primaryDocument.src/app/ceo/workers/[workerId]/edit/page.tsx-97-107 (1)
97-107: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve non-string metadata values.
Any metadata value that is not a string becomes
"". The form then submits the empty value, so a stored number or boolean is lost on save. Convert primitives instead of discarding them.🐛 Proposed fix
Object.entries(document.metadata).map(([key, value]) => [ key, - typeof value === "string" ? value : "", + value === null || typeof value === "object" ? "" : String(value), ]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/`[workerId]/edit/page.tsx around lines 97 - 107, Update the metadata normalization expression in the edit form to preserve primitive non-string values instead of replacing them with empty strings. Within the Object.fromEntries mapping, convert numbers and booleans to their string representation while retaining existing string values and the current fallback for unsupported values.src/components/phase3/worker-detail/document-list.tsx-149-153 (1)
149-153: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle
nullmetadata explicitly.The hidden field sends the literal string
"null"whendocument.metadataisnull, anddocumentMetadataSchemathen rejects it becauseZ.record()requires an object. If this submission path can sendnull, make the schema accept an optionalmetadataJSON blob, or send{}instead of"null".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/document-list.tsx` around lines 149 - 153, Update the hidden metadata field in the document detail submission flow to avoid serializing null as the string "null": use an empty object when document.metadata is null, while preserving existing metadata serialization for non-null values.
🧹 Nitpick comments (20)
src/lib/phase7/import-workbook.test.ts (1)
9-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new document-number requirement and identifier normalization.
This fixture sets
expectsDocumentNumber: true, but no test adds aWorkerDocumentsrow with a blankDocument Numberto confirm the new validation issue inimport-workbook.ts(Line 511-512) fires. No test also exercisesnormalizedIdentifier's new behavior of stripping all non-alphanumeric characters (Line 141), for example matching an existing identifier likeAB-123456against a workbook valueAB123456.Add two cases:
- A
WorkerDocumentsrow for a document type withexpectsDocumentNumber: trueand a blankDocument Number, asserting the resulting issue.- A
lookup.existingWorkerIdentifiersentry containing a dash, matched against a workbook CNIC/Passport with the dash removed, asserting the duplicate-identifier issue fires.Do you want me to draft these test cases?
As per path instructions, "Use the smallest test set that protects changed behavior; add or update tests for critical interactions, permission boundaries, calculations, routing behavior, or regression-prone state, but not purely cosmetic details."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase7/import-workbook.test.ts` around lines 9 - 22, Extend the tests around the existing workbook import fixture to cover both changed behaviors: add a WorkerDocuments row for a document type requiring a number with a blank Document Number and assert the corresponding validation issue, and add a dashed value to lookup.existingWorkerIdentifiers while importing the same identifier without punctuation, asserting the duplicate-identifier issue. Use the existing import test helpers and issue assertions.Source: Path instructions
src/lib/phase2/data.ts (1)
452-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the worker-ID filter into a shared helper.
getAuditEntryCountandgetAuditEntriesboth sanitizeoptions.workerIdwith the same regex and build the same.or()clause. Extract this into one helper function, for exampleapplyWorkerIdFilter(query, workerId), and call it from both places. This removes the duplication and keeps the sanitization rule in one location if it needs to change later.♻️ Proposed refactor
+function applyWorkerIdFilter<T>(query: T & { or: (clause: string) => T }, workerId: string): T { + const sanitized = workerId.replace(/[^a-f0-9-]/gi, ""); + return query.or( + `entity_id.eq.${sanitized},before_data->>worker_id.eq.${sanitized},after_data->>worker_id.eq.${sanitized}`, + ); +}Then replace both inline blocks with
if (options.workerId) query = applyWorkerIdFilter(query, options.workerId);.Also applies to: 494-499
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase2/data.ts` around lines 452 - 457, Extract the duplicated worker-ID sanitization and OR-clause construction from getAuditEntryCount and getAuditEntries into a shared applyWorkerIdFilter helper. Have both methods call this helper when options.workerId is present, preserving the existing query behavior while centralizing the regex and filter format.e2e/phase3.spec.ts (1)
135-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the duplicate-confirmation checkbox by its accessible name.
page.getByRole("checkbox")matches every checkbox on the Review stage. Playwright strict mode fails the test if a second checkbox appears later. Use the accessible name of the confirmation control.💚 Proposed fix
- await page.getByRole("checkbox").check(); + await page + .getByRole("checkbox", { name: /deliberately continue|confirm/i }) + .check();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/phase3.spec.ts` around lines 135 - 136, Update the checkbox selection in the Review-stage flow to target the duplicate-confirmation control by its accessible name rather than using the broad getByRole("checkbox") locator; leave the Create worker button interaction unchanged.src/components/phase3/worker-record-form/stages.tsx (1)
280-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant conditional expressions.
Both branches of
current.photoId ? "keep" : "keep"return"keep". The expression on line 284 and the expression on line 297 therefore always evaluate to the same value.♻️ Proposed simplification
setValues((current) => ({ ...current, - photoAction: file ? "replace" : current.photoId ? "keep" : "keep", + photoAction: file ? "replace" : "keep", photoFile: file, }));setValues((current) => ({ ...current, - photoAction: current.photoId ? "keep" : "keep", + photoAction: "keep", photoFile: null, }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-record-form/stages.tsx` around lines 280 - 300, Simplify the photoAction assignments in the file input onChange handler and the photo removal Button callback by replacing the redundant current.photoId conditional expressions with the constant "keep" value, while preserving the existing photoFile updates.supabase/migrations/20260803090000_worker_record_overhaul.sql (1)
1-15: 🧹 Nitpick | 🔵 TrivialPlan the lock impact of this migration.
Squawk reports that the new check constraints and indexes take blocking locks, that the generated column forces a table rewrite, and that dropping
NOT NULLand dropping columns break older clients. Supabase runs each migration file inside one transaction, socreate index concurrentlyis not available here. Two options remain. Split the lock-heavy statements into a separate, non-transactional migration, or accept the maintenance window and record it in the migration notes.Also confirm the deploy order. The dropped
workerscolumns break any application version that still selects them, so deploy the application first.
[operational]Also applies to: 138-148, 295-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260803090000_worker_record_overhaul.sql` around lines 1 - 15, Update the migration plan for document_types_system_code_format, document_types_metadata_fields_array, and document_types_system_code_unique to account for blocking locks; either move lock-heavy changes into a separate non-transactional migration or document the required maintenance window, since this migration runs transactionally and cannot use concurrent index creation. Confirm deployment ordering so the application is deployed before the migration removes workers columns, preserving compatibility with older clients.Source: Linters/SAST tools
src/types/database.ts (1)
2083-2110: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRegenerate
src/types/database.tsfrom the Supabase schema.The
public.Functionskeys are not in alphabetical order:find_worker_identity_duplicateappears beforedecide_leave_request, andsave_worker_recordappears beforesave_worker_document_metadata. Generated type files can be regenerated from the migrated database, and the Supabase CLI output sorts RPC keys alphabetically. Use that output to keep the manual type definitions aligned with the schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/database.ts` around lines 2083 - 2110, Regenerate the public.Functions definitions in database.ts from the current Supabase schema so RPC keys follow the CLI’s alphabetical ordering. Ensure decide_leave_request precedes find_worker_identity_duplicate and save_worker_document_metadata precedes save_worker_record, without changing function signatures or return types.supabase/tests/phase_3_workers_documents.sql (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the negative assertions so unrelated failures do not pass the test.
Both blocks catch
othersand only re-raise the sentinel exception. Any other error, including a wrong argument list or an unrelated constraint violation, makes the test pass silently. Assert the expected error text or sqlstate instead.♻️ Suggested tightening
begin perform public.save_worker_record('', 'Missing identity', '+60120000000', '', 'Malaysia', '32000000-0000-0000-0000-000000000001', '33000000-0000-0000-0000-000000000001', 1000, 0, '', '[]'::jsonb, false); raise exception 'Missing identity should fail'; - exception when others then - if sqlerrm = 'Missing identity should fail' then raise; end if; + exception when raise_exception then + if sqlerrm = 'Missing identity should fail' then raise; end if; + if sqlerrm not like '%identity%' then + raise exception 'Unexpected failure for missing identity: %', sqlerrm; + end if; end;Apply the same change to the duplicate-document block on lines 51-56.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/tests/phase_3_workers_documents.sql` around lines 44 - 56, Update the exception handlers in the missing-identity and duplicate-document blocks around public.save_worker_record so they explicitly verify the expected error message or SQLSTATE, and re-raise any unrelated exception. Ensure wrong arguments or other constraint failures cannot make either negative test pass silently.src/components/phase3/worker-detail/section-picker.test.tsx (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid asserting a utility class.
toHaveClass("w-full")couples the test to a Tailwind class name. Assert observable behavior instead, for example that the mobile trigger and every section link render, so a class rename does not fail the test.As per coding guidelines: "Use the smallest test set that protects changed behavior; add or update tests for critical interactions, permission boundaries, calculations, routing behavior, or regression-prone state, but not purely cosmetic details."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/section-picker.test.tsx` at line 22, Replace the utility-class assertion on trigger in the section-picker test with assertions of observable behavior, such as rendering the mobile trigger and every section link. Keep the test focused on changed interaction or rendering behavior rather than the presentational Tailwind class name.Source: Coding guidelines
src/components/phase3/worker-detail/index.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe barrel mixes the client module with server modules.
./section-pickerdeclares"use client", while./document-listand./primitivesdo not. Consumers that only need a server primitive still import the client module through this barrel. ImportWorkerSectionPickerdirectly from./section-pickerin the pages that render it, and keep the barrel for the server-safe modules.As per coding guidelines: "Use Server Components by default and add
"use client"only at the smallest interactive boundary."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/index.ts` around lines 1 - 3, Update the worker-detail barrel to export only the server-safe modules document-list and primitives, and remove its section-picker export. In the pages that render WorkerSectionPicker, import it directly from ./section-picker so the client boundary remains limited to the interactive component.Source: Coding guidelines
src/app/ceo/workers/actions.ts (3)
74-93: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReturn an action error instead of throwing on the duplicate lookup failure.
findDuplicatethrows when the RPC fails. The throw escapes the server action and the form shows a generic runtime error instead of the handled message pattern used elsewhere in this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/actions.ts` around lines 74 - 93, Update findDuplicate to return the action’s established error result when the find_worker_identity_duplicate RPC fails instead of throwing a new Error. Preserve the existing worker_duplicate_lookup_failed logging and successful result handling, and match the handled message/result pattern used elsewhere in the surrounding action.
121-148: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the CEO role before the request body is processed.
saveWorkerRecordparses the form data and validates every uploaded file, then callsgetCeoContext()at Line 150. Move the role check to the start of the function so unauthenticated callers cannot drive the parsing and file-validation path.🔒️ Proposed fix
): Promise<Phase3ActionState> { + const { supabase } = await getCeoContext(); const id = workerId ? uuidSchema.safeParse(workerId) : null; const result = schema.safeParse(workerInput(formData));- const { supabase } = await getCeoContext(); const duplicate = await findDuplicate(As per coding guidelines: "Every protected operation must verify a valid Clerk session, an active application user, an allowed
CEOorFOREMANrole, and project scope where applicable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/actions.ts` around lines 121 - 148, Update saveWorkerRecord to call getCeoContext and verify the required CEO role before parsing workerId, processing formData, or validating uploaded files; reject unauthorized callers through the existing authorization/error path, while preserving the current validation flow for authorized users.Source: Coding guidelines
286-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the empty branch with an inverted condition.
The
if (preflightFailures.has("photo"))block contains only a comment. Skip the upload with a guard instead.♻️ Proposed refactor
const photo = formData.get("photoFile"); - if (photoAction === "replace" && photo instanceof File && photo.size > 0) { - if (preflightFailures.has("photo")) { - // Metadata is already safely committed; the Review warning offers retry. - } else { - const upload = await uploadWorkerFile({ - file: photo, - kind: "PHOTO", - supabase, - workerId: savedWorkerId, - }); - if (!upload.ok) - failures.push({ clientKey: "photo", message: upload.message }); - } - } + // Metadata is already committed; a failed preflight is reported in Review for retry. + if ( + photoAction === "replace" && + photo instanceof File && + photo.size > 0 && + !preflightFailures.has("photo") + ) { + const upload = await uploadWorkerFile({ + file: photo, + kind: "PHOTO", + supabase, + workerId: savedWorkerId, + }); + if (!upload.ok) + failures.push({ clientKey: "photo", message: upload.message }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/actions.ts` around lines 286 - 300, Invert the preflight check in the photo upload flow so uploadWorkerFile runs only when preflightFailures does not contain "photo". Remove the empty if branch and preserve the existing upload failure handling.src/lib/phase3/validation.test.ts (1)
37-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert each required field separately.
The negative case changes seven fields at the same time and asserts one aggregate
false. A single field could stop being required and the test would still pass. Iterate over one override per field to keep the guarantee per field.♻️ Proposed refactor
- expect( - createWorkerSchema.safeParse({ - ...validWorker, - legalName: "", - phoneNumber: "", - nationality: "", - hourlyRate: "0", - tradeId: "", - skillLevelId: "", - foodDeduction: "-1", - }).success, - ).toBe(false); + const invalidOverrides = [ + { legalName: "" }, + { phoneNumber: "" }, + { nationality: "" }, + { hourlyRate: "0" }, + { tradeId: "" }, + { skillLevelId: "" }, + { foodDeduction: "-1" }, + ]; + for (const override of invalidOverrides) { + expect( + createWorkerSchema.safeParse({ ...validWorker, ...override }).success, + ).toBe(false); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase3/validation.test.ts` around lines 37 - 51, Update the test case around createWorkerSchema to validate each required field independently: iterate through individual overrides for legalName, phoneNumber, nationality, hourlyRate, tradeId, skillLevelId, and foodDeduction, merging one override at a time into validWorker and asserting safeParse(...).success is false. Keep the existing validWorker success assertion and address-optional behavior unchanged.src/app/ceo/workers/[workerId]/page.tsx (2)
148-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated duration formatter.
src/app/foreman/workers/[workerId]/page.tsxLines 97-99 define the identicalminuteshelper. Move it into a shared format module so both pages format attendance durations the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/`[workerId]/page.tsx around lines 148 - 150, Extract the duplicated minutes helper into the shared format module, then update the minutes usages in the CEO and foreman worker pages to import and reuse that shared formatter. Preserve the existing hours-and-minutes output behavior.
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two imports from
@/lib/phase3/data.Lines 24-28 and Line 48 import from the same module. Add
listAssignableProjectsto the first import statement.♻️ Proposed fix
import { getWorkerCore, getWorkerForSection, getWorkerIdentity, + listAssignableProjects, } from "`@/lib/phase3/data`"; @@ -import { listAssignableProjects } from "`@/lib/phase3/data`";Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/ceo/workers/`[workerId]/page.tsx around lines 24 - 28, Merge the separate imports from "`@/lib/phase3/data`" into one import declaration. Update the existing import containing getWorkerCore, getWorkerForSection, and getWorkerIdentity to also include listAssignableProjects, then remove the duplicate module import.src/components/phase3/worker-detail/primitives.tsx (1)
54-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey
InfoRowsrows by index instead of by stringified label.
String(label)returns[object Object]for element labels, which produces duplicate keys. All current callers pass strings, so this is defensive only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/phase3/worker-detail/primitives.tsx` around lines 54 - 60, Update the row key generation in InfoRows to use each row’s array index instead of String(label). Keep the existing label/value rendering and layout unchanged, ensuring element labels cannot produce duplicate React keys.src/lib/phase4/data.ts (1)
427-437: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding the assignment lookup to the requested month.
The query returns every assignment the worker ever had, so
projectIdscan include projects with no activity in the month. Filtering by the month window reduces theinlists on the four follow-up queries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase4/data.ts` around lines 427 - 437, Update the worker_project_assignments lookup in the assignment query to restrict results to assignments overlapping the requested month window, using the existing month start/end values and assignment date fields. Keep the existing error handling and projectIds deduplication, so only projects relevant to the requested month reach the follow-up queries.src/lib/phase3/data.ts (3)
322-330: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBound the identifier set used for pagination.
The search collects every matching worker id and later passes the set to
.in("id", matchingIds). On a large workforce this produces a very long request URL and repeats the work on the recursive page-clamp call at Lines 349-359. Consider a database-side join or view for search, or cap the collected ids.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase3/data.ts` around lines 322 - 330, Bound the worker IDs accumulated in relatedFilters before intersectWorkerIds is called, preventing oversized .in("id", matchingIds) requests and repeated work during recursive pagination clamping. Update the search flow around profileMatches, documentMatches, and intersectWorkerIds to enforce a safe maximum while preserving the existing intersection behavior for retained IDs.
575-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse a map for system-code lookups.
Both mappings call
findon the document-type array inside amapover documents. AMapkeyed by type id already exists at Line 546 for names. Build one map that holds the whole type record and readnameandsystem_codefrom it.Also applies to: 819-822
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase3/data.ts` around lines 575 - 579, Reuse a single document-type Map keyed by type id in the document mapping logic. Build it from the complete type records, then read both name and system_code from the mapped record instead of calling documentTypes.data.find inside each documents.map, updating both occurrences of documentTypeSystemCode and the existing name lookup.
840-840: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
getWorkerForTabalias.Only
getWorkerForSectionis imported or called outsidesrc/lib/phase3/data.ts. Keep the section-based API as the single entry point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/phase3/data.ts` at line 840, Remove the exported getWorkerForTab alias and retain getWorkerForSection as the sole worker lookup API in src/lib/phase3/data.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 759d7d2b-8720-4d7c-9beb-a4514b2cb6e9
⛔ Files ignored due to path filters (25)
docs/design-references/worker-record-overhaul/00-concept-board.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/01-create-personal-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/01-create-personal-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/02-create-documents-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/02-create-documents-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/03-create-review-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/03-create-review-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/04-create-flow-mobile.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/04-create-flow-mobile.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/05-edit-review-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/05-edit-review-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/06-edit-review-mobile.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/06-edit-review-mobile.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/07-worker-overview-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/07-worker-overview-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/08-worker-documents-desktop.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/08-worker-documents-desktop.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/09-worker-overview-mobile.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/09-worker-overview-mobile.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/10-worker-documents-mobile.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/10-worker-documents-mobile.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/11-worker-attendance-mobile.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/11-worker-attendance-mobile.svgis excluded by!**/*.svgdocs/design-references/worker-record-overhaul/12-mobile-section-picker.pngis excluded by!**/*.pngdocs/design-references/worker-record-overhaul/12-mobile-section-picker.svgis excluded by!**/*.svg
📒 Files selected for processing (58)
docs/design-references/worker-record-overhaul/README.mde2e/phase3.spec.tse2e/support/phase4-database.tse2e/support/phase5-database.tse2e/support/phase6-database.tssrc/app/api/imports/preview/route.tssrc/app/api/imports/uploads/route.tssrc/app/api/workers/[workerId]/documents/[documentId]/route.tssrc/app/api/workers/[workerId]/documents/route.tssrc/app/ceo/settings/page.tsxsrc/app/ceo/workers/[workerId]/edit/loading.tsxsrc/app/ceo/workers/[workerId]/edit/page.tsxsrc/app/ceo/workers/[workerId]/page.tsxsrc/app/ceo/workers/actions.tssrc/app/ceo/workers/new/loading.tsxsrc/app/ceo/workers/new/page.tsxsrc/app/ceo/workers/page.tsxsrc/app/foreman/workers/[workerId]/page.tsxsrc/app/foreman/workers/page.tsxsrc/components/operations/loading-skeletons.tsxsrc/components/operations/route-loading.tsxsrc/components/phase3/worker-detail/document-list.tsxsrc/components/phase3/worker-detail/index.tssrc/components/phase3/worker-detail/primitives.tsxsrc/components/phase3/worker-detail/section-picker.test.tsxsrc/components/phase3/worker-detail/section-picker.tsxsrc/components/phase3/worker-form.tsxsrc/components/phase3/worker-record-form/document-editor.tsxsrc/components/phase3/worker-record-form/form.test.tsxsrc/components/phase3/worker-record-form/form.tsxsrc/components/phase3/worker-record-form/helpers.test.tssrc/components/phase3/worker-record-form/helpers.tssrc/components/phase3/worker-record-form/index.tssrc/components/phase3/worker-record-form/review-summary.tsxsrc/components/phase3/worker-record-form/stages.tsxsrc/components/phase3/worker-record-form/stepper.tsxsrc/components/phase3/worker-record-form/types.tssrc/components/phase7/import-workspace.tsxsrc/lib/phase2/audit.test.tssrc/lib/phase2/audit.tssrc/lib/phase2/data.tssrc/lib/phase3/data.tssrc/lib/phase3/file-storage.tssrc/lib/phase3/files.test.tssrc/lib/phase3/files.tssrc/lib/phase3/validation.test.tssrc/lib/phase3/validation.tssrc/lib/phase4/data.tssrc/lib/phase7/import-workbook.test.tssrc/lib/phase7/import-workbook.tssrc/lib/phase7/reports.tssrc/types/database.tssupabase/migrations/20260803090000_worker_record_overhaul.sqlsupabase/tests/phase_3_workers_documents.sqlsupabase/tests/phase_4_offline_attendance.sqlsupabase/tests/phase_5_leave.sqlsupabase/tests/phase_6_payroll.sqlsupabase/tests/phase_7_reports_imports.sql
| insert into public.worker_documents (worker_id, file_kind, document_type_id, document_number) | ||
| select worker_id, 'DOCUMENT', id, 'E2E-PHASE-4' | ||
| from public.document_types where system_code = 'PASSPORT'; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The three E2E fixtures insert no passport document when the PASSPORT document type is absent. Each fixture uses insert into public.worker_documents ... select ... from public.document_types where system_code = 'PASSPORT'. If the seed lacks that row, the statement inserts zero rows, setup reports success, and the test fails later with an unrelated symptom. Worker identity now lives in worker_documents, so a missing document silently changes what each phase exercises. Resolve the document type into a variable first, and raise an exception when it is null.
e2e/support/phase4-database.ts#L133-L136: add apassport_type_id uuiddeclaration, select it before the insert, raise an exception when it is null, then insert with'E2E-PHASE-4'.e2e/support/phase5-database.ts#L121-L124: apply the same lookup and guard, then insert with'E2E-PHASE-5'.e2e/support/phase6-database.ts#L153-L156: apply the same lookup and guard, then insert with'E2E-PHASE-6'.
🐛 Proposed pattern (shown for Phase 4)
declare
ceo_id uuid;
project_id uuid;
+ passport_type_id uuid;
worker_id uuid := gen_random_uuid();
begin
@@
- insert into public.worker_documents (worker_id, file_kind, document_type_id, document_number)
- select worker_id, 'DOCUMENT', id, 'E2E-PHASE-4'
- from public.document_types where system_code = 'PASSPORT';
+ select id into passport_type_id
+ from public.document_types
+ where system_code = 'PASSPORT';
+
+ if passport_type_id is null then
+ raise exception 'Phase 4 E2E requires a PASSPORT document type';
+ end if;
+
+ insert into public.worker_documents (
+ worker_id, file_kind, document_type_id, document_number
+ )
+ values (worker_id, 'DOCUMENT', passport_type_id, 'E2E-PHASE-4');Also confirm that worker_documents has no NOT NULL created_by or updated_by column. These inserts omit both, and session_replication_role = replica does not bypass NOT NULL checks.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 3 files
e2e/support/phase4-database.ts#L133-L136(this comment)e2e/support/phase5-database.ts#L121-L124e2e/support/phase6-database.ts#L153-L156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/support/phase4-database.ts` around lines 133 - 136, Update the
worker_documents fixture inserts in e2e/support/phase4-database.ts:133-136,
e2e/support/phase5-database.ts:121-124, and
e2e/support/phase6-database.ts:153-156 to declare and populate a
passport_type_id uuid, raise an exception when the PASSPORT document type is
missing, then insert using that variable and the existing phase-specific
document number. Also verify worker_documents does not require NOT NULL
created_by or updated_by values; make no insert change for those columns unless
the schema requires it.
| let removedDocumentIds: string[] = []; | ||
| try { | ||
| const parsed = JSON.parse( | ||
| String(formData.get("removedDocumentIds") ?? "[]"), | ||
| ); | ||
| if (Array.isArray(parsed)) { | ||
| removedDocumentIds = parsed.filter( | ||
| (value): value is string => uuidSchema.safeParse(value).success, | ||
| ); | ||
| } | ||
| } catch { | ||
| removedDocumentIds = []; | ||
| } | ||
| if (removedDocumentIds.length > 0) { | ||
| const removedDocuments = await supabase | ||
| .from("worker_documents") | ||
| .select("bucket_id,object_path") | ||
| .in("id", removedDocumentIds) | ||
| .eq("status", "REMOVED"); | ||
| for (const document of removedDocuments.data ?? []) { | ||
| await bestEffortStorageCleanup({ | ||
| bucketId: document.bucket_id, | ||
| objectPath: document.object_path, | ||
| supabase, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the removed-document lookup to the saved worker.
removedDocumentIds comes from the client form. The query filters only by id and status, so any REMOVED document id from another worker also reaches bestEffortStorageCleanup and its stored object is deleted. Add a worker_id filter.
🔒️ Proposed fix
const removedDocuments = await supabase
.from("worker_documents")
.select("bucket_id,object_path")
.in("id", removedDocumentIds)
+ .eq("worker_id", savedWorkerId)
.eq("status", "REMOVED");📝 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.
| let removedDocumentIds: string[] = []; | |
| try { | |
| const parsed = JSON.parse( | |
| String(formData.get("removedDocumentIds") ?? "[]"), | |
| ); | |
| if (Array.isArray(parsed)) { | |
| removedDocumentIds = parsed.filter( | |
| (value): value is string => uuidSchema.safeParse(value).success, | |
| ); | |
| } | |
| } catch { | |
| removedDocumentIds = []; | |
| } | |
| if (removedDocumentIds.length > 0) { | |
| const removedDocuments = await supabase | |
| .from("worker_documents") | |
| .select("bucket_id,object_path") | |
| .in("id", removedDocumentIds) | |
| .eq("status", "REMOVED"); | |
| for (const document of removedDocuments.data ?? []) { | |
| await bestEffortStorageCleanup({ | |
| bucketId: document.bucket_id, | |
| objectPath: document.object_path, | |
| supabase, | |
| }); | |
| } | |
| } | |
| let removedDocumentIds: string[] = []; | |
| try { | |
| const parsed = JSON.parse( | |
| String(formData.get("removedDocumentIds") ?? "[]"), | |
| ); | |
| if (Array.isArray(parsed)) { | |
| removedDocumentIds = parsed.filter( | |
| (value): value is string => uuidSchema.safeParse(value).success, | |
| ); | |
| } | |
| } catch { | |
| removedDocumentIds = []; | |
| } | |
| if (removedDocumentIds.length > 0) { | |
| const removedDocuments = await supabase | |
| .from("worker_documents") | |
| .select("bucket_id,object_path") | |
| .in("id", removedDocumentIds) | |
| .eq("worker_id", savedWorkerId) | |
| .eq("status", "REMOVED"); | |
| for (const document of removedDocuments.data ?? []) { | |
| await bestEffortStorageCleanup({ | |
| bucketId: document.bucket_id, | |
| objectPath: document.object_path, | |
| supabase, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/ceo/workers/actions.ts` around lines 237 - 263, Update the
removed-document query in the worker action to add a worker_id filter matching
the saved worker before calling bestEffortStorageCleanup. Keep the existing id
and REMOVED status filters, and use the worker identifier already available in
the surrounding save flow.
Source: Coding guidelines
| <form | ||
| action={`/api/workers/${worker.id}/documents`} | ||
| method="post" | ||
| encType="multipart/form-data" | ||
| className="grid gap-3 rounded-lg border border-slate-200 p-3" | ||
| > | ||
| <input type="hidden" name="intent" value="save" /> | ||
| <input type="hidden" name="fileKind" value="DOCUMENT" /> | ||
| <input | ||
| type="hidden" | ||
| name="replaceDocumentId" | ||
| value={document.id} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="documentTypeId" | ||
| value={document.document_type_id ?? ""} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="documentNumber" | ||
| value={document.document_number ?? ""} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="issueDate" | ||
| value={document.issue_date ?? ""} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="expiryDate" | ||
| value={document.expiry_date ?? ""} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="metadata" | ||
| value={JSON.stringify(document.metadata)} | ||
| /> | ||
| <label className="grid gap-2 text-sm font-medium"> | ||
| {document.object_path | ||
| ? "Replace file" | ||
| : "Attach file"} | ||
| <input | ||
| required | ||
| name="file" | ||
| type="file" | ||
| accept={workerDocumentAccept} | ||
| className="min-h-11 rounded-lg border border-slate-300 p-2" | ||
| /> | ||
| </label> | ||
| <button className="min-h-11 rounded-lg border border-violet-200 text-sm font-semibold text-violet-800"> | ||
| {document.object_path | ||
| ? "Replace file" | ||
| : "Attach file"} | ||
| </button> | ||
| </form> | ||
| {document.object_path ? ( | ||
| <form | ||
| action={`/api/workers/${worker.id}/documents`} | ||
| method="post" | ||
| > | ||
| <input | ||
| type="hidden" | ||
| name="intent" | ||
| value="remove-file" | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="documentId" | ||
| value={document.id} | ||
| /> | ||
| <button className="min-h-11 w-full rounded-lg border border-amber-300 text-sm font-semibold text-amber-900"> | ||
| Remove file only | ||
| </button> | ||
| </form> | ||
| ) : null} | ||
| <form | ||
| action={`/api/workers/${worker.id}/documents`} | ||
| method="post" | ||
| > | ||
| <input | ||
| type="hidden" | ||
| name="intent" | ||
| value="remove-document" | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="documentId" | ||
| value={document.id} | ||
| /> | ||
| <button className="min-h-11 w-full rounded-lg border border-red-300 text-sm font-semibold text-red-800"> | ||
| Remove document | ||
| </button> | ||
| </form> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the document route handler for origin or token verification.
fd -p 'api/workers' -t f | xargs -r rg -n -C4 'origin|Origin|Sec-Fetch|referer|Referer|csrf|intent'Repository: SherryMaster/worksite-operations-platform
Length of output: 5245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Next docs availability =="
fd -p 'next/dist/docs' -t f | head -20 || true
echo
echo "== document list relevant section =="
sed -n '1,230p' src/components/phase3/worker-detail/document-list.tsx
echo
echo "== document route handler =="
sed -n '1,120p' 'src/app/api/workers/[workerId]/documents/route.ts'
echo
echo "== authentication helpers and middleware related refs =="
rg -n 'NextAuth|clerk|Auth|auth|cookies|secure|sameSite|httpOnly|middleware|verify|Origin|Sec-Fetch-Site|X-CSRF|csrf' src/app src/lib src/components src/middleware.ts 2>/dev/null | head -200Repository: SherryMaster/worksite-operations-platform
Length of output: 24694
🌐 Web query:
Next.js Route Handlers form POST cookie auth cross-site origin check Sec-Fetch-Site documentation
💡 Result:
Next.js Route Handlers do not include the automatic CSRF (Cross-Site Request Forgery) protections—such as origin/host collation—that are built into Next.js Server Actions [1][2]. When using cookie-based authentication for POST or other state-changing methods in a Route Handler, you must implement manual security checks to prevent CSRF [1][3]. Key Security Considerations and Implementation: 1. Manual CSRF Protection: Since Route Handlers are exposed as standard API endpoints, they do not automatically verify that a request originated from your own application [1][4]. For mutations (POST, PUT, PATCH, DELETE), you should verify the request origin to ensure it matches your expected host [1][2]. 2. Using Fetch Metadata (Sec-Fetch-Site): A recommended, modern approach is to inspect the Sec-Fetch-Site header [5][6]. This header informs the server about the context of the request [6]. You can implement a policy to reject state-changing requests if the Sec-Fetch-Site is 'cross-site' [5]. Example implementation in a Route Handler: import { headers } from 'next/headers'; export async function POST(request: Request) { const headersList = await headers; const fetchSite = headersList.get('sec-fetch-site'); // Reject cross-site POST requests if (fetchSite === 'cross-site') { return new Response('Forbidden: Cross-site request', { status: 403 }); } // Proceed with authentication and business logic... } 3. Cookie Configuration: Always secure your session cookies using the following attributes to mitigate CSRF and other vulnerabilities [7][4]: - SameSite: Set to 'Lax' (default in most modern browsers) or 'Strict' for high-security applications to prevent cookies from being sent in cross-site contexts [8][7][3]. - HttpOnly: Prevents client-side scripts from accessing the cookie [8][7]. - Secure: Ensures the cookie is only sent over encrypted HTTPS connections [9][7]. 4. Authentication Defense-in-Depth: Never rely solely on middleware to protect Route Handlers [10][1]. Middleware may not cover every path, and it cannot prevent direct access from non-browser clients [1][4]. Always perform an explicit authentication and authorization check inside the Route Handler itself before processing any data [10][1][4]. 5. Alternative (Server Actions): If your application logic is primarily triggered by UI interactions, consider using Server Actions instead of Route Handlers [11]. Server Actions are designed to include automatic CSRF protections and are generally preferred for data mutations originating from your Next.js application [1][2][11].
Citations:
- 1: https://mudassirworks.hashnode.dev/next-js-authentication-security-patterns-that-hold-up-in-production
- 2: https://tomodahinata.com/en/blog/nextjs-csrf-origin-protection-server-actions-guide
- 3: https://securie.ai/guides/csrf-in-next-js
- 4: https://workos.com/blog/nextjs-app-router-authentication-guide-2026
- 5: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
- 6: https://web.dev/articles/fetch-metadata
- 7: https://en.nextjs.im/docs/pages/guides/authentication/
- 8: https://www.authgear.com/post/nextjs-session-management/
- 9: https://nextjs.org/docs/app/api-reference/functions/cookies
- 10: https://www.authgear.com/post/nextjs-api-route-authentication/
- 11: https://github.com/vercel/nextjs-skills/blob/HEAD/skills/next-best-practices/route-handlers.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant document route sections =="
sed -n '60,180p' 'src/app/api/workers/[workerId]/documents/route.ts'
sed -n '1,120p' 'src/lib/auth/access.ts'
echo
echo "== all cross-site mutation Route Handlers with explicit fetch metadata/origin checks =="
python3 - <<'PY'
import subprocess, pathlib, re
files = subprocess.check_output(['git','ls-files','src/app/api','src/app/**/*.ts','src/app/**/*.tsx'], text=True, stderr=subprocess.DEVNULL).splitlines()
words = {
"no_csrf_mutations": [],
}
patterns = [
re.compile(r"POST\s*\("),
re.compile(r"(PUT|PATCH|DELETE)\s*\("),
re.compile(r"fmt\.RequireSameOrigin"),
re.compile(r"(Origin|origin|Sec-Fetch-Site|null-origin|same-site|cross-site|csrf|Token|token)"),
]
for path in files:
p = pathlib.Path(path)
if not p.exists():
continue
text = p.read_text()
if not any(text.find(pat.group()+")")!=-1 or text.find(pat.group())!=-1 for pat in patterns[:3]):
continue
has_mut = bool(patterns[0].search(text) or patterns[1].search(text))
has_check = bool(patterns[2].search(text) or re.search(r"fmt\.RequireSameOrigin|req\.Referer|req\.Origin|Sec-Fetch-Site|SameSite|_csrf|csrf", text))
has_token = bool(re.search(r"csrf|CSRF|X-CSRF|Token|token|headers\.get\s*\(\s*[\"']x-csrf|secret|signature|signature", text, re.I))
if has_mut and not has_check and not has_token:
words["no_csrf_mutations"].append(path)
for k,v in words.items():
print(f"-- {k} --")
for path in sorted(v)[:50]:
print(path)
PYRepository: SherryMaster/worksite-operations-platform
Length of output: 5523
Add cross-site request protection to these Route Handler form posts.
src/components/phase3/worker-detail/document-list.tsx sends authenticated POST forms to src/app/api/workers/[workerId]/documents/route.ts, including destructive remove-file and remove-document intents. The Route Handler validates Clerk role access but does not reject cross-site requests. Add an explicit check such as Sec-Fetch-Site: same-site, validate Origin, require a CSRF token, or move the mutations to Server Actions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/phase3/worker-detail/document-list.tsx` around lines 116 -
209, Add CSRF protection to the POST forms in the document-list component,
covering the save, remove-file, and remove-document intents. Ensure each
authenticated mutation includes a verifiable same-site signal or CSRF token, and
update the documents Route Handler to reject requests that fail this validation
before processing the intents.
| basePath: string; | ||
| sections: WorkerSection[]; | ||
| }) { | ||
| const selected = sections.find((section) => section.value === active)!; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the non-null assertion on selected.
If active matches no entry in sections, selected is undefined and Line 57 throws during render. The assertion hides this from the type checker. Fall back to the first section.
🛡️ Proposed fix
- const selected = sections.find((section) => section.value === active)!;
+ const selected =
+ sections.find((section) => section.value === active) ?? sections[0];
+ if (!selected) return null;Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/phase3/worker-detail/section-picker.tsx` at line 28, Update
the selected-section lookup in the section picker to remove the non-null
assertion and fall back to the first entry in sections when active matches
nothing, ensuring render-time access to selected remains safe.
| const normalizedQuery = query?.trim().replace(/[%_,]/g, ""); | ||
| if (normalizedQuery) { | ||
| const pattern = `%${normalizedQuery}%`; | ||
| const [profileMatches, documentMatches] = await Promise.all([ | ||
| supabase | ||
| .from("workers") | ||
| .select("id") | ||
| .or(`legal_name.ilike.${pattern},phone_number.ilike.${pattern}`), | ||
| supabase | ||
| .from("worker_documents") | ||
| .select("worker_id") | ||
| .eq("status", "ACTIVE") | ||
| .ilike("document_number", pattern), | ||
| ]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgREST or filter escaping parentheses in ilike value
💡 Result:
To filter using the ilike operator in PostgREST when your pattern contains reserved characters such as parentheses (or commas, dots, colons, and asterisks), you must wrap the pattern in double quotes [1][2]. Because PostgREST reserves certain characters for its URL grammar, any pattern containing them will be incorrectly parsed unless quoted [1][2]. To apply this: 1. Wrap the value in double quotes: ilike."(your-pattern-with-parentheses)" [1][2]. 2. Use percent-encoding if required by your client/environment (e.g., %22 for double quotes) [1][2]. If your pattern itself contains a double quote character, you must escape it with a backslash (") [1][2]. Backslashes themselves are escaped with a double backslash (\) [1][2]. Example syntax (unencoded for clarity):?column=ilike."(pattern)" If using a client library, such as the PostgREST JavaScript client, ensure you are using a version that automatically handles this quoting and escaping for you [3]. Recent updates to client libraries have improved support to ensure patterns containing reserved characters are wrapped in double quotes and correctly escaped [3].
Citations:
- 1: https://docs.postgrest.org/en/stable/references/api/url_grammar.html
- 2: https://docs.postgrest.org/en/latest/references/api/url_grammar.html
- 3: fix(postgrest): escape " and \ inside quoted filter values supabase/supabase-js#2529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant Supabase query code and surrounding error handling.
if [ -f src/lib/phase3/data.ts ]; then
sed -n '270,340p' src/lib/phase3/data.ts | cat -n
printf '\n--- nearby throwQueryError usages ---\n'
rg -n "throwQueryError|createWorker" src/lib/phase3/data.ts src || true
fi
printf '\n--- package / docs availability ---\n'
[ -d node_modules/next/dist/docs ] && find node_modules/next/dist/docs -maxdepth 2 -type f | head -20 || trueRepository: SherryMaster/worksite-operations-platform
Length of output: 19117
Escape the search pattern before using it in PostgREST filters.
The worker profile or() filter and document ilike() filter build the pattern from user input. Parentheses, quotes, backslashes, and other expression-delimiter characters can break parsing or alter the search results. Strip those characters or apply PostgREST quoting/escaping before constructing the filters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/phase3/data.ts` around lines 302 - 315, Update the query
normalization before constructing pattern in the phase 3 search flow to escape
or remove PostgREST expression-delimiter characters, including parentheses,
quotes, and backslashes, from user input. Ensure the sanitized pattern is used
by both the workers `or()` filter and worker_documents `ilike()` filter while
preserving the existing wildcard search behavior.
| insert into public.document_types ( | ||
| name, | ||
| system_code, | ||
| expects_document_number, | ||
| expects_issue_date, | ||
| expects_expiry_date, | ||
| metadata_fields, | ||
| created_by, | ||
| updated_by | ||
| ) | ||
| select | ||
| seed.name, | ||
| seed.system_code, | ||
| seed.expects_document_number, | ||
| seed.expects_issue_date, | ||
| seed.expects_expiry_date, | ||
| seed.metadata_fields, | ||
| application_users.id, | ||
| application_users.id | ||
| from ( | ||
| values | ||
| ( | ||
| 'CNIC', 'CNIC', true, false, false, | ||
| '["issuingCountry"]'::jsonb | ||
| ), | ||
| ( | ||
| 'Passport', 'PASSPORT', true, true, true, | ||
| '["issuingCountry"]'::jsonb | ||
| ), | ||
| ( | ||
| 'Work Permit', 'WORK_PERMIT', true, true, true, | ||
| '["permitType","issuingAuthority","employerSponsor"]'::jsonb | ||
| ), | ||
| ( | ||
| 'CIDB Construction Personnel Registration', | ||
| 'CIDB_REGISTRATION', | ||
| true, | ||
| true, | ||
| true, | ||
| '["registrationCategory"]'::jsonb | ||
| ), | ||
| ( | ||
| 'Safety/Health Induction Certificate', | ||
| 'SAFETY_CERTIFICATE', | ||
| true, | ||
| true, | ||
| true, | ||
| '["certificateType","provider"]'::jsonb | ||
| ), | ||
| ( | ||
| 'FOMEMA/Medical Fitness Certificate', | ||
| 'MEDICAL_CERTIFICATE', | ||
| true, | ||
| true, | ||
| true, | ||
| '["providerClinic","examinationDate"]'::jsonb | ||
| ), | ||
| ( | ||
| 'i-Kad / foreign-worker identity card', | ||
| 'IKAD', | ||
| true, | ||
| true, | ||
| true, | ||
| '["sectorCardType"]'::jsonb | ||
| ), | ||
| ( | ||
| 'Employment Contract', | ||
| 'EMPLOYMENT_CONTRACT', | ||
| false, | ||
| true, | ||
| true, | ||
| '["employer","contractStartDate","contractEndDate"]'::jsonb | ||
| ), | ||
| ( | ||
| 'Other', 'OTHER', false, false, false, | ||
| '["issuer","notes"]'::jsonb | ||
| ) | ||
| ) as seed( | ||
| name, | ||
| system_code, | ||
| expects_document_number, | ||
| expects_issue_date, | ||
| expects_expiry_date, | ||
| metadata_fields | ||
| ) | ||
| cross join lateral ( | ||
| select id | ||
| from public.application_users | ||
| where role = 'CEO'::public.application_role | ||
| order by created_at | ||
| limit 1 | ||
| ) application_users | ||
| on conflict (lower(btrim(name))) do update | ||
| set | ||
| system_code = excluded.system_code, | ||
| expects_document_number = excluded.expects_document_number, | ||
| expects_issue_date = excluded.expects_issue_date, | ||
| expects_expiry_date = excluded.expects_expiry_date, | ||
| metadata_fields = excluded.metadata_fields; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the unique expression index on document_types and any CEO seed.
rg -n 'document_types' supabase/migrations -g '*.sql' | rg -n 'unique|index'
rg -n "role = 'CEO'|'CEO'::public.application_role" supabase/migrations -g '*.sql'Repository: SherryMaster/worksite-operations-platform
Length of output: 1307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Migration snippets:\n'
for f in supabase/migrations/20260803090000_worker_record_overhaul.sql supabase/migrations/20260724030000_phase_3_workers_documents.sql; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
wc -l "$f"
sed -n '1,180p' "$f" | cat -n
fi
done
printf '\nCandidate unique indexes on document_types:\n'
rg -n 'create|unique index|document_types_name_unique' supabase/migrations -g '*.sql'Repository: SherryMaster/worksite-operations-platform
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
migrations = sorted(Path('supabase/migrations').glob('*.sql'))
for i, idx, stmt, line in [
(path, line_no, stmt.strip(), line_no + j)
for path in migrations
for j, line in enumerate(path.read_text().splitlines(), 1)
for stmt in re.findall(r'create unique index .*?\n\s*\(|create unique index .*?;', line, flags=re.I)
]:
try:
print(stmt)
except Exception:
print(f'ERROR parsing {path}:{line_no+1}')
raise
PYRepository: SherryMaster/worksite-operations-platform
Length of output: 179
Add the missing CEO guard before inserting document types.
This migration creates document_types_system_code_unique before the upsert, but the insert ... select only runs when a CEO row exists. If no CEO row is present, the cross join lateral returns no rows, and the migration succeeds without inserting the new document types. Add a guard before the insert, and fail explicitly when no CEO row exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260803090000_worker_record_overhaul.sql` around lines
38 - 136, Add an explicit CEO existence guard immediately before the
document-types insert, using the same CEO lookup criteria as the lateral join;
raise a migration error when no eligible application_users row exists, then
leave the existing upsert unchanged for databases with a CEO.
| add constraint worker_documents_kind_requirements | ||
| check ( | ||
| ( | ||
| file_kind = 'PHOTO' | ||
| and document_type_id is null | ||
| and bucket_id = 'worker-photos' | ||
| and mime_type in ( | ||
| 'image/jpeg', 'image/png', 'image/webp', 'image/heic', 'image/heif' | ||
| ) | ||
| ) | ||
| or ( | ||
| file_kind = 'DOCUMENT' | ||
| and document_type_id is not null | ||
| and (bucket_id is null or bucket_id = 'worker-documents') | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
remove_worker_document can create a PHOTO row that violates worker_documents_kind_requirements.
When p_remove_document is false and the document is a PHOTO with a file, the elsif branch inserts a replacement row that copies current_document.file_kind but sets no file columns. The worker_documents_kind_requirements constraint requires bucket_id = 'worker-photos' and a non-null mime_type for file_kind = 'PHOTO', and the alternative branch requires file_kind = 'DOCUMENT'. The insert therefore raises a constraint violation.
This path is reachable. src/app/api/workers/[workerId]/documents/route.ts selects the document by id and status = 'ACTIVE' without filtering file_kind, and passes p_remove_document: intent !== "remove-file". A remove-file intent that carries a photo document id reaches this branch.
🐛 Proposed fix: restrict the file-only removal branch to documents
- elsif current_document.bucket_id is not null then
+ elsif current_document.bucket_id is not null
+ and current_document.file_kind = 'DOCUMENT' then
replacement_id := gen_random_uuid();Add a preceding branch for photos so a file-only removal on a photo removes the row instead:
elsif current_document.file_kind = 'PHOTO' then
update public.worker_documents set status = 'REMOVED', changed_by = actor_id
where id = current_document.id;Also applies to: 957-975
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 206-221: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260803090000_worker_record_overhaul.sql` around lines
206 - 221, Update remove_worker_document so the file-only removal path handles
PHOTO rows before the replacement-row branch: when p_remove_document is false
and current_document.file_kind = 'PHOTO', mark the existing row REMOVED with
actor_id instead of inserting a file-less PHOTO replacement. Preserve the
existing replacement behavior for DOCUMENT rows.
| update public.worker_documents | ||
| set | ||
| document_type_id = (input_document ->> 'documentTypeId')::uuid, | ||
| document_number = nullif(btrim(input_document ->> 'documentNumber'), ''), | ||
| issue_date = nullif(input_document ->> 'issueDate', '')::date, | ||
| expiry_date = nullif(input_document ->> 'expiryDate', '')::date, | ||
| metadata = coalesce(input_document -> 'metadata', '{}'::jsonb), | ||
| changed_by = actor_id | ||
| where id = target_document_id | ||
| and worker_id = target_worker_id | ||
| and file_kind = 'DOCUMENT' | ||
| and status = 'ACTIVE'; | ||
|
|
||
| if not found then | ||
| insert into public.worker_documents ( | ||
| id, | ||
| worker_id, | ||
| file_kind, | ||
| document_type_id, | ||
| document_number, | ||
| issue_date, | ||
| expiry_date, | ||
| metadata | ||
| ) values ( | ||
| target_document_id, | ||
| target_worker_id, | ||
| 'DOCUMENT', | ||
| (input_document ->> 'documentTypeId')::uuid, | ||
| nullif(btrim(input_document ->> 'documentNumber'), ''), | ||
| nullif(input_document ->> 'issueDate', '')::date, | ||
| nullif(input_document ->> 'expiryDate', '')::date, | ||
| coalesce(input_document -> 'metadata', '{}'::jsonb) | ||
| ); | ||
| end if; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The if not found then insert fallback can raise a duplicate-key error. Both RPCs update worker_documents with a filter on status = 'ACTIVE', then insert with the same id when no row matched. A row with that id can still exist with status = 'REPLACED' or status = 'REMOVED', because attach_worker_file and remove_worker_document set those states and keep the original id. A stale client id then produces a primary-key violation instead of a clear message.
supabase/migrations/20260803090000_worker_record_overhaul.sql#L680-L713: insave_worker_record, detect an existing non-ACTIVErow fortarget_document_idand raise an explicit exception, such asThe document metadata is no longer current, or allocate a new id withgen_random_uuid()before the insert.supabase/migrations/20260803090000_worker_record_overhaul.sql#L894-L912: apply the same handling insave_worker_document_metadatafortarget_id.
📍 Affects 1 file
supabase/migrations/20260803090000_worker_record_overhaul.sql#L680-L713(this comment)supabase/migrations/20260803090000_worker_record_overhaul.sql#L894-L912
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260803090000_worker_record_overhaul.sql` around lines
680 - 713, In save_worker_record at
supabase/migrations/20260803090000_worker_record_overhaul.sql lines 680-713,
handle an existing non-ACTIVE worker_documents row for target_document_id before
the fallback insert by raising an explicit stale-document exception or
allocating a new UUID; apply the same handling in save_worker_document_metadata
at lines 894-912 for target_id, ensuring neither path can trigger a
duplicate-key error.
1 participant
Summary
Main areas
src/components/phase3/worker-record-form/andsrc/components/phase3/worker-detail/supabase/migrations/20260803090000_worker_record_overhaul.sqland affected tests/typesScope
Validation is delegated to GitHub CI per repository instructions.
Summary by CodeRabbit