fix: 4 critical bugs — data corruption, data loss, and silent state mutation#6
Draft
cursor[bot] wants to merge 5 commits intomainfrom
Draft
fix: 4 critical bugs — data corruption, data loss, and silent state mutation#6cursor[bot] wants to merge 5 commits intomainfrom
cursor[bot] wants to merge 5 commits intomainfrom
Conversation
The isLegacyContractConstraintOrSchemaError function had a missing
parentheses issue where `lower.includes('status')` alone (without
the && conditions) would match almost any Postgres error containing
the word 'status'. This caused unrelated errors (RLS, permission,
constraint) to be silently routed through the legacy fallback path,
potentially saving contracts with a reduced/corrupted schema.
Removed the overly broad `includes('status')` check entirely. The
remaining conditions (contracts_status_check constraint name, and
missing-column pattern) are precise enough for the intended fallback.
Co-authored-by: Siddhartha Dwivedi <info@siddart.net>
The deleteContract fallback path ran invoice unlink (setting contract_id to null) for ANY first-delete error, not just FK constraint violations. This could clear invoice-contract links even when the contract was not actually deleted (e.g. RLS denied the delete, or a transient error occurred). Now the fallback only triggers when the error message matches foreign-key constraint patterns. Other errors are thrown immediately. Co-authored-by: Siddhartha Dwivedi <info@siddart.net>
…eneration generateInvoiceShareLink was unconditionally setting status='unpaid' when generating or refreshing a share link. This silently reverted paid or archived invoices back to unpaid — corrupting invoice state and breaking income tracking. Changed to read-only SELECT to get the existing share_token and only UPDATE with a new token when one is missing, without touching status. Co-authored-by: Siddhartha Dwivedi <info@siddart.net>
Guest invoices were never migrated to Supabase during sign-up. migrateGuestData migrated clients, contracts, and time entries, then called clearAll() which wiped all guest data including invoices. Any guest who created invoices before signing up would silently lose them. Added invoice migration step (step 3) with client_id remapping and computed tax_amount, matching the pattern used by the other entity migrations. Co-authored-by: Siddhartha Dwivedi <info@siddart.net>
Verifies the operator-precedence fix: ensures the function does NOT match generic errors containing 'status' in non-constraint contexts, while still correctly matching the two legitimate legacy patterns. Co-authored-by: Siddhartha Dwivedi <info@siddart.net>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Automated deep inspection found 4 critical bugs that cause data corruption, data loss, or silent state mutation in production paths.
Bugs Fixed
1. Operator precedence bug silently corrupts contract saves (
lib/api/contracts.ts)Bug:
isLegacyContractConstraintOrSchemaErrorhad|| lower.includes('status')without parentheses, which — due to&&binding tighter than||— matched any Postgres error containing the word "status". This sent unrelated errors (RLS violations, permission errors, other constraint failures) through the legacy fallback path, silently saving contracts with a reduced schema (missing milestones, clauses, template_type, etc.).Trigger: Any contract create/update that fails with an error message containing "status" (e.g. RLS policy mentioning status, enum validation errors).
Fix: Removed the overly broad
includes('status')clause. The remaining two patterns (contracts_status_checkandcolumn … contracts … does not exist) are specific enough.2. Contract delete unlinks invoices even on non-FK errors (
lib/api/contracts.ts)Bug:
deleteContractran the invoice-unlink fallback (contract_id = null) for any first-delete error, not just foreign-key constraint violations. If the delete failed due to RLS, permissions, or transient errors, invoices would still have theircontract_idcleared — even though the contract was never deleted.Trigger: Delete fails for any reason other than FK constraints (e.g. RLS denies delete but allows invoice update).
Fix: Added FK-specific error pattern matching. Non-FK errors are now thrown immediately without running the unlink.
3. Share link generation resets paid invoices to unpaid (
lib/api/invoices.ts)Bug:
generateInvoiceShareLinkunconditionally setstatus: 'unpaid'when generating a share link. Sharing a paid or archived invoice silently reverted its payment status, corrupting income tracking data.Trigger: User generates/refreshes a share link for a paid invoice.
Fix: Changed initial query from
UPDATEtoSELECT(read-only). TheUPDATEonly runs in the fallback path when ashare_tokenneeds to be generated, and no longer touchesstatus.4. Guest invoices lost on signup (
lib/migration/migrateGuestData.ts)Bug:
migrateGuestDatamigrated clients, contracts, and time entries, then calledclearAll()which wiped all guest data including invoices — but invoices were never migrated. Any guest user who created invoices before signing up would silently lose them.Trigger: Guest creates invoices, then signs up. All invoices are permanently deleted.
Fix: Added invoice migration step (step 3) with client_id remapping and computed
tax_amount, following the same pattern as the other entity migrations.Validation
tsc --noEmit)