Assets register, migration backlog fixes, and mobile/tablet responsive fixes - #167
Merged
Merged
Conversation
Adds a standalone, company-wide register to track fixed assets (computers, printers, vehicles, equipment) and investments (crypto, stocks) with acquisition cost, current value, and disposal tracking. Deliberately does not post to the ledger/Chart of Accounts in v1 - it's record-keeping only, following the existing billing_items table pattern for company-wide (non-division-scoped) records. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Widen the New Asset dialog (max-w-md -> max-w-2xl) and put Type/Name on the same row so the form reads less cramped. - Add migration 0038 to rename the tender_schedule_* enums, tables, columns, indexes, and FK constraints to project_schedule_*/ project_progress_*, matching what packages/db/src/schema/project-schedule.ts has declared for a while. The rename had previously been applied to the dev database out-of-band (e.g. via drizzle-kit push) without a matching migration ever being committed, which left the tracked snapshot metadata stale and made `bun db:generate` fail with an unresolvable interactive prompt for any future schema change. All statements are existence-guarded so the migration is a safe no-op wherever the rename was already applied, and rewrites cleanly from scratch on a fresh database. Verified `bun db:generate` completes without prompting after this migration is applied. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds an asset_valuations table so investments can record a value at a point in time instead of only overwriting a single current_value. Recording a valuation keeps assets.current_value synced to the latest entry, and the asset detail page shows a Valuation History table (with per-entry growth vs. the prior entry) plus an overall Growth % in the summary sidebar (current value vs. cost). Still manual record-keeping - no automated pricing/market data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds an asset_transactions table (deposit | withdrawal) so investments can record top-ups and partial/full sales after the initial deposit, not just one-way growth. Each transaction can optionally move the held quantity too (buying/selling units), which stays synced onto assets.quantity. Total Invested (shown in the asset detail sidebar and used for the Growth % calc) is now cost + deposits - withdrawals. Also relabels the "Cost" field to "Initial Deposit" for investments in the New Asset dialog, since that's what it represents once deposits/withdrawals exist. Verified end-to-end in the browser: created a fixed asset and an investment, added a deposit and a withdrawal (quantity and Total Invested updated correctly), recorded a valuation (Growth % matched expected calculation), disposed/reactivated, and deleted both - cleaned up test data afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add assets register for fixed assets and investments
bun db:generate was surfacing a mix of pending schema changes every time it ran: a compliance_documents table, leads.company_name, and three billing_line_items discount columns/checks. Investigation showed the table, both columns, and the two discount value columns already existed on the dev database (created out-of-band, e.g. via drizzle-kit push) - only the three CHECK constraints on billing_line_items were genuinely missing, and the tracked snapshot metadata had never caught up either way. Adds migration 0041 with existence-guarded statements (safe no-op wherever a piece was already applied, does the real thing on a fresh database) and corrects the snapshot chain to match. Verified `bun db:migrate` applies cleanly and `bun db:generate` no longer surfaces this backlog (the only remaining diff is a pre-existing, unrelated FK-identifier-truncation cosmetic quirk in Drizzle-kit itself, left untouched). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix compliance_documents/leads/billing-discount migration backlog
- deleteAsset now blocks (with a clear error message) when the asset has any recorded valuations or deposit/withdrawal transactions, since those cascade-delete silently otherwise. Mirrors the existing billing_items pattern of steering users to archive/dispose instead of delete when a record is in use. - Give the project_progress_sections/items foreign keys short, explicit names. Drizzle's default FK name for these two exceeded Postgres's 63-byte identifier limit and got silently truncated on creation, which meant `bun db:generate` reported a spurious drop+recreate diff every single time it ran. Migration 0042 renames the constraints in place (guarded, safe no-op if already applied). `bun db:generate` now reports "No schema changes, nothing to migrate" on a clean checkout. Verified end-to-end: created an investment, added a valuation, confirmed delete was blocked with the history still intact; deleted the valuation and confirmed the asset then deleted cleanly. Confirmed the FK rename applied and `db:generate` is fully silent afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Invoices card on a division's detail page listed every invoice with both Total and Balance columns, including fully-paid ones (shown with a "-" balance). Drop the Total column and add an onlyOutstanding filter to getAllInvoices() (issued/overdue/ partially_paid, matching the status set already used for the "outstanding" sum) so fully-paid invoices don't clutter the list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Full responsive pass (mobile 375px, tablet 768px) across all nav
pages surfaced two related bugs:
1. Root cause: SidebarInset (apps/admin/src/components/ui/sidebar.tsx)
is a flex item with no min-w-0, so any sufficiently wide content
inside <main> forced the whole layout column (including TopNav)
wider than the viewport instead of scrolling internally - the
classic "flexbox children default to min-width: auto" trap. Fixed
at the shared component and defensively on the app layout's <main>.
2. Several horizontally-scrolling tab/filter strips relied on
`overflow-x-auto` that was inert without a `min-w-0` (or, in one
case, an explicit `shrink-0`) on the same or an ancestor flex/grid
item, so the strip's content just kept the whole page wide instead
of scrolling in place:
- LeadStatusTabs (leads page status filter)
- SettingsNav (top-level settings section switcher)
- Settings layout's <aside> wrapping SettingsNav - this was the
actual remaining offender after fixing SettingsNav itself,
confirmed via getBoundingClientRect() sweep
- org-settings-form.tsx's Company Identity/Contact/Address/Logo
tabs (no overflow handling at all previously)
- data/page.tsx and security/page.tsx tabs (same, no overflow
handling)
- reports-tabs.tsx (Overview/Revenue/Expenses/Net Profit strip)
- client-billing-workspace.tsx's document tabs (had `shrink-0`,
which forces full content width and defeats overflow-x-auto
entirely)
3. Separately, dashboard-shell.tsx's period selector (Current Month/
Previous Month/Year to Date) showed full labels starting at the
`sm` breakpoint (640px) in a 3-column grid, overlapping at tablet
width (768px) where the combined label width didn't fit. Pushed
the full-label breakpoint to `lg` (1024px) so tablet keeps the
existing short labels (Current/Previous/YTD), and added min-w-0 +
truncate as a backstop.
Verified via document.body.scrollWidth === viewport width (no
horizontal page overflow) across Leads, Settings (all 5 sub-pages),
Reports, a client detail page, and the Dashboard, at both mobile and
tablet widths, plus visual screenshots at each. No console or server
errors; type-check and lint clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…k-names Guard asset delete, fix FK naming noise, and trim division invoices card
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughChangesAsset Register
Admin layout and billing refinements
Database maintenance migrations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant AssetsPage
participant AssetEditClient
participant assetsActions
participant AssetQueries
participant Database
Admin->>AssetsPage: open asset register
AssetsPage->>AssetQueries: load filtered assets and summary
AssetQueries->>Database: query asset records
Database-->>AssetQueries: return assets and totals
AssetQueries-->>AssetsPage: render asset register
Admin->>AssetEditClient: save or change asset status
AssetEditClient->>assetsActions: submit normalized asset data
assetsActions->>AssetQueries: persist validated mutation
AssetQueries->>Database: update asset state
Database-->>AssetQueries: return mutation result
assetsActions-->>AssetEditClient: return success or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
apps/admin/src/app/(admin)/assets/[id]/page.tsx (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
generateMetadatato show the asset name in the page title.The static title is
'Asset'for every asset.generateMetadatareceives the sameparamspromise and can load the asset name.🤖 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 `@apps/admin/src/app/`(admin)/assets/[id]/page.tsx at line 18, Replace the static metadata export with an async generateMetadata function that awaits the route params, loads the corresponding asset, and uses its name in the page title. Preserve the existing Metadata return shape and handle the asset lookup through the page’s established data-access path.apps/admin/src/app/(admin)/assets/assets-table.tsx (2)
83-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe empty-state branches are unreachable.
apps/admin/src/app/(admin)/assets/page.tsxrendersEmptyStatewhenitems.length === 0and only rendersAssetsTablefor a non-empty list. Keep these branches only if you plan to reuseAssetsTableelsewhere.Also applies to: 97-101
🤖 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 `@apps/admin/src/app/`(admin)/assets/assets-table.tsx around lines 83 - 89, Remove the unreachable empty-state TableRow branches from the AssetsTable component, including both assets.length === 0 and any corresponding loading/empty branch near the referenced code. Keep page.tsx’s EmptyState handling unchanged and preserve AssetsTable rendering for non-empty asset lists.
16-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the database row type instead of redeclaring it.
AssetRowrestates the asset shape with looser types (kind: string,status: string). The exportedAssetRowtype inpackages/db/src/queries/assets.tsalready models this row and keeps the enum unions. Import it, and derive aPick<...>for the fields this component needs.🤖 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 `@apps/admin/src/app/`(admin)/assets/assets-table.tsx around lines 16 - 27, Remove the local AssetRow interface and import the exported AssetRow type from the database assets queries module. Define the component’s row type as a Pick of that imported type containing only the fields used by assets-table, preserving the database type’s enum unions for kind and status.apps/admin/src/app/(admin)/assets/page.tsx (1)
53-87: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe stat cards always show active totals, including in the disposed view.
getAssetsSummaryaggregates active assets only. Whenstatus=disposedis selected, the cards still report active values. Consider labelling the cards as active-only, or recomputing them for the selected filter.🤖 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 `@apps/admin/src/app/`(admin)/assets/page.tsx around lines 53 - 87, Update the summary cards in the assets page to reflect the selected status filter: when viewing disposed assets, use a summary computed for disposed records rather than the active-only result from getAssetsSummary. Ensure the displayed labels and values consistently describe the selected filter, while preserving the existing active view behavior.apps/admin/src/app/actions/assets-actions.ts (3)
90-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the caught error.
The catch blocks discard the error object. Nine actions in this file return a generic message with no record of the cause. Diagnosing a failed insert then requires reproduction.
Capture the error and log it with the action name and the asset id.
🤖 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 `@apps/admin/src/app/actions/assets-actions.ts` around lines 90 - 92, Update the catch blocks in the asset action functions to capture the caught error and log it with the specific action name and asset id before returning the existing generic error response. Apply this consistently to all nine actions in assets-actions.ts without changing their returned messages.
26-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the date format, not just the string length.
acquisitionDate,valuationDate, andtransactionDateonly checkmin(10). A value such as"not-a-date"passes validation and reaches thedatecolumn. The insert then throws, and the catch block returns the generic message "Failed to save. Please try again.". The user gets no useful reason.Add a format check so validation rejects the value with a clear message.
♻️ Proposed fix
-const isoDate = z.string().regex(/^\d{4}-\d{2}-\d{2}$/, 'Use a valid date (YYYY-MM-DD).');Apply it in each schema:
- acquisitionDate: z.string().min(10, 'Acquisition date is required'), + acquisitionDate: isoDate,- valuationDate: z.string().min(10, 'Date is required'), + valuationDate: isoDate,- transactionDate: z.string().min(10, 'Date is required'), + transactionDate: isoDate,Also applies to: 44-44, 53-53
🤖 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 `@apps/admin/src/app/actions/assets-actions.ts` at line 26, Update the Zod schemas for acquisitionDate, valuationDate, and transactionDate to validate an actual date format rather than only requiring a 10-character string. Add a clear format-specific validation message to each field while preserving the existing required-field validation.
107-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared field mapping.
Lines 108-119 duplicate lines 74-85 exactly. A change to one mapping can miss the other and cause divergent create and update behaviour.
♻️ Proposed refactor
+function toAssetColumns(v: AssetInput) { + const isFixed = v.kind === 'fixed_asset'; + const isInvestment = v.kind === 'investment'; + return { + kind: v.kind, + name: v.name, + category: v.category, + acquisitionDate: v.acquisitionDate, + cost: v.cost.toFixed(2), + currentValue: v.currentValue != null ? v.currentValue.toFixed(2) : null, + notes: v.notes ?? null, + serialNumber: isFixed ? (v.serialNumber ?? null) : null, + location: isFixed ? (v.location ?? null) : null, + assignedTo: isFixed ? (v.assignedTo ?? null) : null, + quantity: isInvestment && v.quantity != null ? String(v.quantity) : null, + unitType: isInvestment ? (v.unitType ?? null) : null, + }; +}Then call
await updateAssetRow(id, toAssetColumns(v));andawait createAssetRow(toAssetColumns(v));.🤖 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 `@apps/admin/src/app/actions/assets-actions.ts` around lines 107 - 120, Extract the duplicated asset field mapping into a shared toAssetColumns function, preserving the existing conditional and formatting behavior. Replace the inline mapping in both the create and update flows with toAssetColumns(v), calling createAssetRow and updateAssetRow with its result.apps/admin/src/app/(admin)/assets/[id]/transaction-history.tsx (1)
191-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the quantity value.
t.quantityis a numeric column and arrives as a string with the full column scale, for example"0.50000000". The cell renders it unchanged, while the adjacent amount cell usesformatZAR. The column is hard to read.Convert and format the value, for example
Number(t.quantity).toLocaleString('en-ZA', { maximumFractionDigits: 8 }).🤖 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 `@apps/admin/src/app/`(admin)/assets/[id]/transaction-history.tsx around lines 191 - 193, Update the quantity rendering in the transaction history table to convert t.quantity from its string representation and format it for readability using the en-ZA locale with up to 8 fractional digits, while preserving the '-' fallback for nullish values.apps/admin/src/app/(admin)/assets/add-asset-dialog.tsx (1)
123-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd responsive prefixes to the grid columns.
These rows use fixed
grid-cols-3andgrid-cols-2at every viewport width. On a narrow screen the type select and the name input share a 3-column row and become cramped. The PR targets mobile and tablet responsiveness, andtransaction-history.tsxalready usessm:prefixes for its form layout.Stack the fields on small screens.
♻️ Proposed fix
- <div className="grid grid-cols-3 gap-3"> + <div className="grid grid-cols-1 sm:grid-cols-3 gap-3">- <div className="grid grid-cols-2 gap-3"> + <div className="grid grid-cols-1 sm:grid-cols-2 gap-3">Apply the second change to the rows at lines 154, 183, 224, and 254. Keep
col-span-2guarded withsm:col-span-2on lines 139 and 243.Also applies to: 154-154, 183-183, 224-224, 254-254
🤖 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 `@apps/admin/src/app/`(admin)/assets/add-asset-dialog.tsx at line 123, Make the form grid rows in the add-asset dialog responsive by using single-column layouts on small screens and applying the existing multi-column layouts from the sm breakpoint onward. Update the rows containing grid-cols-3 or grid-cols-2 at the referenced locations, and ensure the related col-span-2 classes use sm:col-span-2 so fields stack without cramped spans on narrow screens.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/admin/src/app/`(admin)/assets/[id]/asset-edit-client.tsx:
- Line 35: Update the useTransition call in the asset edit client to retain its
pending flag, combine it with the existing mutation state as the shared busy
value, and remove the manual setIsSubmitting updates from handleSave,
handleDispose, handleReactivate, and handleDelete. Use busy for every action
button’s disabled prop and the “Saving…” label so all mutations prevent repeated
submissions while running.
- Around line 126-233: Associate every form label in the asset edit form with
its control by adding matching htmlFor and id attributes, following the pattern
used in add-asset-dialog.tsx. Update the Name, Category, Acquisition Date, Cost,
Current Value, Serial Number, Location, Assigned To, Quantity, Unit Type, and
Notes controls, and add an aria-label to the SelectTrigger for Type.
In `@apps/admin/src/app/`(admin)/assets/add-asset-dialog.tsx:
- Around line 72-73: Validate every parsed numeric value with Number.isFinite
before negative-value checks: update add-asset-dialog.tsx lines 72-73,
asset-edit-client.tsx lines 57, 71, and 76, transaction-history.tsx lines 51-52
and 59, and valuation-history.tsx lines 39-40. In add-asset-dialog.tsx, also
preserve an entered quantity of 0 instead of converting it to undefined at line
91.
In `@apps/admin/src/app/`(admin)/assets/assets-table.tsx:
- Around line 60-65: Make the desktop asset rows keyboard-accessible by adding a
focusable navigation control, preferably a Link in the asset name cell,
targeting `/assets/${asset.id}`. Update the TableRow rendering in the assets map
while preserving the existing click navigation and visual behavior.
In `@apps/admin/src/app/`(admin)/assets/page.tsx:
- Around line 20-21: Normalize status once in the assets page before
constructing filter links, using a statusFilter value that preserves only the
supported disposed state and otherwise represents active assets. Update the
kind-link query parameters around the existing link construction to use
statusFilter instead of the raw status, including all occurrences near lines
100–116, while keeping showDisposed based on the normalized value.
In
`@apps/admin/src/app/`(admin)/relationships/clients/[id]/client-billing-workspace.tsx:
- Line 983: Update the overflowing TabsList in
apps/admin/src/app/(admin)/relationships/clients/[id]/client-billing-workspace.tsx:983-983
and apps/admin/src/components/reports/reports-tabs.tsx:109-109 to include
justify-start in their class lists, ensuring both horizontally scrolling tab
lists start-align their tabs.
In `@apps/admin/src/app/actions/assets-actions.ts`:
- Around line 224-243: Update addAssetTransaction to load the asset after
validation and reject the request when its kind is not an investment before
calling addAssetTransactionRow. Return the action’s existing error shape for
non-investment assets, and preserve the current insert mapping for valid
investments.
- Around line 63-93: Move getSessionOrRedirect() in createAsset before the try
block so its NEXT_REDIRECT exception is not intercepted by the catch. Keep
validation, asset creation, revalidation, and existing save-error handling
inside the try block.
- Around line 164-168: Enforce the asset-history deletion rule at the database
boundary rather than relying only on the application-level hasAssetHistory
check. Update the asset foreign-key definitions to use ON DELETE RESTRICT/NO
ACTION, or modify the delete flow around deleteAssetRow to lock the asset row
and perform the history check and deletion within one transaction; ensure
concurrent valuations or deposits/withdrawals cannot be cascaded away.
In
`@packages/db/src/migrations/0038_rename_tender_schedule_to_project_schedule.sql`:
- Around line 10-78: Scope every existence guard in
packages/db/src/migrations/0038_rename_tender_schedule_to_project_schedule.sql
(lines 10-78) to public: filter pg_type and information_schema.columns by
schema, join pg_constraint through its owning relation and require the expected
public table, and qualify ALTER TABLE/ALTER TYPE targets with public. Apply the
same public-table constraint guard in
packages/db/src/migrations/0042_shorten_project_progress_fk_names.sql (lines
7-13), covering each constraint rename.
In `@packages/db/src/migrations/0041_compliance_leads_discounts.sql`:
- Around line 20-24: Scope every pg_constraint lookup in migration 0041 by
target table: add conrelid = 'compliance_documents'::regclass to the
compliance_documents foreign-key guard at lines 20-24, and add conrelid =
'billing_line_items'::regclass to each discount-constraint guard at lines 33-47.
In `@packages/db/src/queries/assets.ts`:
- Around line 164-177: Wrap the paired history and asset-update statements in
db.transaction for addAssetValuation, addAssetTransaction, and
deleteAssetTransaction. Execute both operations through the transaction-scoped
database handle so they commit or roll back together and concurrent updates on
the same asset are serialized; preserve each function’s existing return values
and validation behavior.</code>
- Around line 239-248: Update the quantity adjustment logic in
deleteAssetTransaction and the corresponding transaction update block so an
over-withdrawal cannot be silently clamped by greatest(..., 0). Reject
withdrawals exceeding the held quantity, or persist and reuse the actual applied
delta so deletion reverses exactly the recorded change; preserve normal deposits
and valid withdrawals.
- Around line 80-85: Update reactivateAsset to also clear the asset’s
disposalNotes when resetting status and disposedAt, ensuring reactivated assets
do not retain notes from a previous disposal.
In `@packages/db/src/queries/billing.ts`:
- Around line 557-559: Update the query conditions used by the onlyOutstanding
filter to require a strictly positive remaining balance, calculated as invoice
total minus allocatedAmount, in addition to the existing outstanding-status
predicate. Ensure this predicate is added to the shared conditions array so both
returned invoices and the outstanding total exclude fully allocated and
overallocated records, and add regression coverage for both cases.
---
Nitpick comments:
In `@apps/admin/src/app/`(admin)/assets/[id]/page.tsx:
- Line 18: Replace the static metadata export with an async generateMetadata
function that awaits the route params, loads the corresponding asset, and uses
its name in the page title. Preserve the existing Metadata return shape and
handle the asset lookup through the page’s established data-access path.
In `@apps/admin/src/app/`(admin)/assets/[id]/transaction-history.tsx:
- Around line 191-193: Update the quantity rendering in the transaction history
table to convert t.quantity from its string representation and format it for
readability using the en-ZA locale with up to 8 fractional digits, while
preserving the '-' fallback for nullish values.
In `@apps/admin/src/app/`(admin)/assets/add-asset-dialog.tsx:
- Line 123: Make the form grid rows in the add-asset dialog responsive by using
single-column layouts on small screens and applying the existing multi-column
layouts from the sm breakpoint onward. Update the rows containing grid-cols-3 or
grid-cols-2 at the referenced locations, and ensure the related col-span-2
classes use sm:col-span-2 so fields stack without cramped spans on narrow
screens.
In `@apps/admin/src/app/`(admin)/assets/assets-table.tsx:
- Around line 83-89: Remove the unreachable empty-state TableRow branches from
the AssetsTable component, including both assets.length === 0 and any
corresponding loading/empty branch near the referenced code. Keep page.tsx’s
EmptyState handling unchanged and preserve AssetsTable rendering for non-empty
asset lists.
- Around line 16-27: Remove the local AssetRow interface and import the exported
AssetRow type from the database assets queries module. Define the component’s
row type as a Pick of that imported type containing only the fields used by
assets-table, preserving the database type’s enum unions for kind and status.
In `@apps/admin/src/app/`(admin)/assets/page.tsx:
- Around line 53-87: Update the summary cards in the assets page to reflect the
selected status filter: when viewing disposed assets, use a summary computed for
disposed records rather than the active-only result from getAssetsSummary.
Ensure the displayed labels and values consistently describe the selected
filter, while preserving the existing active view behavior.
In `@apps/admin/src/app/actions/assets-actions.ts`:
- Around line 90-92: Update the catch blocks in the asset action functions to
capture the caught error and log it with the specific action name and asset id
before returning the existing generic error response. Apply this consistently to
all nine actions in assets-actions.ts without changing their returned messages.
- Line 26: Update the Zod schemas for acquisitionDate, valuationDate, and
transactionDate to validate an actual date format rather than only requiring a
10-character string. Add a clear format-specific validation message to each
field while preserving the existing required-field validation.
- Around line 107-120: Extract the duplicated asset field mapping into a shared
toAssetColumns function, preserving the existing conditional and formatting
behavior. Replace the inline mapping in both the create and update flows with
toAssetColumns(v), calling createAssetRow and updateAssetRow with its result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80008db6-37ad-4c72-a8b7-e2d3b99ab0a5
📒 Files selected for processing (41)
apps/admin/src/app/(admin)/assets/[id]/asset-edit-client.tsxapps/admin/src/app/(admin)/assets/[id]/page.tsxapps/admin/src/app/(admin)/assets/[id]/transaction-history.tsxapps/admin/src/app/(admin)/assets/[id]/valuation-history.tsxapps/admin/src/app/(admin)/assets/add-asset-dialog.tsxapps/admin/src/app/(admin)/assets/assets-table.tsxapps/admin/src/app/(admin)/assets/page.tsxapps/admin/src/app/(admin)/layout.tsxapps/admin/src/app/(admin)/relationships/clients/[id]/client-billing-workspace.tsxapps/admin/src/app/(admin)/relationships/divisions/[id]/page.tsxapps/admin/src/app/(admin)/settings/data/page.tsxapps/admin/src/app/(admin)/settings/layout.tsxapps/admin/src/app/(admin)/settings/organisation/org-settings-form.tsxapps/admin/src/app/(admin)/settings/security/page.tsxapps/admin/src/app/actions/assets-actions.tsapps/admin/src/components/dashboard/dashboard-shell.tsxapps/admin/src/components/leads/lead-status-tabs.tsxapps/admin/src/components/navigation/nav-data.tsapps/admin/src/components/reports/reports-tabs.tsxapps/admin/src/components/settings/settings-nav.tsxapps/admin/src/components/ui/sidebar.tsxpackages/db/src/index.tspackages/db/src/migrations/0037_add_assets_register.sqlpackages/db/src/migrations/0038_rename_tender_schedule_to_project_schedule.sqlpackages/db/src/migrations/0039_add_asset_valuations.sqlpackages/db/src/migrations/0040_add_asset_transactions.sqlpackages/db/src/migrations/0041_compliance_leads_discounts.sqlpackages/db/src/migrations/0042_shorten_project_progress_fk_names.sqlpackages/db/src/migrations/meta/0037_snapshot.jsonpackages/db/src/migrations/meta/0038_snapshot.jsonpackages/db/src/migrations/meta/0039_snapshot.jsonpackages/db/src/migrations/meta/0040_snapshot.jsonpackages/db/src/migrations/meta/0041_snapshot.jsonpackages/db/src/migrations/meta/0042_snapshot.jsonpackages/db/src/migrations/meta/_journal.jsonpackages/db/src/queries/assets.tspackages/db/src/queries/billing.tspackages/db/src/queries/index.tspackages/db/src/schema/assets.tspackages/db/src/schema/index.tspackages/db/src/schema/project-schedule.ts
|
|
||
| export function AssetEditClient({ asset }: AssetEditClientProps) { | ||
| const router = useRouter(); | ||
| const [, startTransition] = useTransition(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the pending flag from useTransition.
Line 35 discards the pending value. isSubmitting is set only in handleSave. handleDispose, handleReactivate, and handleDelete never set it, so the buttons on lines 240-270 stay enabled while those actions run. A user can trigger the same mutation several times.
🐛 Proposed fix
- const [, startTransition] = useTransition();
+ const [isPending, startTransition] = useTransition();Then replace the local flag with a combined value and remove the manual setIsSubmitting calls:
- const [isSubmitting, setIsSubmitting] = useState(false);
+ const busy = isPending;Use busy for every disabled prop and for the "Saving…" label.
Also applies to: 89-122
🤖 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 `@apps/admin/src/app/`(admin)/assets/[id]/asset-edit-client.tsx at line 35,
Update the useTransition call in the asset edit client to retain its pending
flag, combine it with the existing mutation state as the shared busy value, and
remove the manual setIsSubmitting updates from handleSave, handleDispose,
handleReactivate, and handleDelete. Use busy for every action button’s disabled
prop and the “Saving…” label so all mutations prevent repeated submissions while
running.
Comment on lines
+126
to
+233
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium"> | ||
| Type <span className="text-destructive">*</span> | ||
| </label> | ||
| <Select value={kind} onValueChange={(v) => setKind(v as AssetKind)} disabled={isSubmitting}> | ||
| <SelectTrigger className="w-full"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="fixed_asset">Fixed Asset</SelectItem> | ||
| <SelectItem value="investment">Investment</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium"> | ||
| Name <span className="text-destructive">*</span> | ||
| </label> | ||
| <Input value={name} onChange={(e) => setName(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Category <span className="text-destructive">*</span></label> | ||
| <Input value={category} onChange={(e) => setCategory(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Acquisition Date <span className="text-destructive">*</span></label> | ||
| <Input | ||
| type="date" | ||
| value={acquisitionDate} | ||
| onChange={(e) => setAcquisitionDate(e.target.value)} | ||
| disabled={isSubmitting} | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Cost <span className="text-destructive">*</span></label> | ||
| <Input | ||
| type="number" | ||
| min="0" | ||
| step="0.01" | ||
| value={cost} | ||
| onChange={(e) => setCost(e.target.value)} | ||
| disabled={isSubmitting} | ||
| /> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Current Value</label> | ||
| <Input | ||
| type="number" | ||
| min="0" | ||
| step="0.01" | ||
| value={currentValue} | ||
| onChange={(e) => setCurrentValue(e.target.value)} | ||
| disabled={isSubmitting} | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| {kind === 'fixed_asset' ? ( | ||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Serial Number</label> | ||
| <Input value={serialNumber} onChange={(e) => setSerialNumber(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Location</label> | ||
| <Input value={location} onChange={(e) => setLocation(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5 col-span-2"> | ||
| <label className="text-sm font-medium">Assigned To</label> | ||
| <Input value={assignedTo} onChange={(e) => setAssignedTo(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Quantity <span className="text-destructive">*</span></label> | ||
| <Input | ||
| type="number" | ||
| min="0" | ||
| step="any" | ||
| value={quantity} | ||
| onChange={(e) => setQuantity(e.target.value)} | ||
| disabled={isSubmitting} | ||
| /> | ||
| </div> | ||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Unit Type <span className="text-destructive">*</span></label> | ||
| <Input value={unitType} onChange={(e) => setUnitType(e.target.value)} disabled={isSubmitting} /> | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| <div className="flex flex-col gap-1.5"> | ||
| <label className="text-sm font-medium">Notes</label> | ||
| <Textarea | ||
| value={notes} | ||
| onChange={(e) => setNotes(e.target.value)} | ||
| rows={3} | ||
| disabled={isSubmitting} | ||
| className="min-h-[80px]" | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Associate each label with its input.
Every <label> in this form lacks htmlFor, and every Input lacks an id. Screen readers announce these controls without a name. A click on the label does not move focus to the control.
add-asset-dialog.tsx already pairs FieldLabel htmlFor with an input id. Use the same pattern here.
♿ Proposed fix, applied to the Name field
- <label className="text-sm font-medium">
+ <label htmlFor="asset-edit-name" className="text-sm font-medium">
Name <span className="text-destructive">*</span>
</label>
- <Input value={name} onChange={(e) => setName(e.target.value)} disabled={isSubmitting} />
+ <Input id="asset-edit-name" value={name} onChange={(e) => setName(e.target.value)} disabled={isSubmitting} />Repeat for Category, Acquisition Date, Cost, Current Value, Serial Number, Location, Assigned To, Quantity, Unit Type, and Notes. Add aria-label to the SelectTrigger on line 131.
🤖 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 `@apps/admin/src/app/`(admin)/assets/[id]/asset-edit-client.tsx around lines
126 - 233, Associate every form label in the asset edit form with its control by
adding matching htmlFor and id attributes, following the pattern used in
add-asset-dialog.tsx. Update the Name, Category, Acquisition Date, Cost, Current
Value, Serial Number, Location, Assigned To, Quantity, Unit Type, and Notes
controls, and add an aria-label to the SelectTrigger for Type.
Comment on lines
+72
to
+73
| const costValue = parseFloat(cost) || 0; | ||
| if (costValue < 0) { toast.error('Cost cannot be negative.'); return; } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unchecked parseFloat results in all four asset forms. Each form parses a numeric input with parseFloat and then tests the result with a < 0 comparison. parseFloat returns NaN for an unparsable value, and NaN < 0 evaluates to false. Every guard passes, and NaN reaches the server action. The Zod schema then rejects the value with a message about a negative number, which does not describe the cause. Add a Number.isFinite check at each site.
apps/admin/src/app/(admin)/assets/add-asset-dialog.tsx#L72-L73: replaceparseFloat(cost) || 0with a parsed value validated byNumber.isFinite, and stop converting a quantity of0toundefinedon line 91.apps/admin/src/app/(admin)/assets/[id]/asset-edit-client.tsx#L57-L57: validate the parsedcostwithNumber.isFinitebefore the< 0test, and apply the same check tocurrentValueandquantityon lines 71 and 76.apps/admin/src/app/(admin)/assets/[id]/transaction-history.tsx#L51-L52: validate the parsedamountwithNumber.isFinite, and validate the parsedquantityon line 59.apps/admin/src/app/(admin)/assets/[id]/valuation-history.tsx#L39-L40: validate the parsedvaluewithNumber.isFinitebefore the< 0test.
📍 Affects 4 files
apps/admin/src/app/(admin)/assets/add-asset-dialog.tsx#L72-L73(this comment)apps/admin/src/app/(admin)/assets/[id]/asset-edit-client.tsx#L57-L57apps/admin/src/app/(admin)/assets/[id]/transaction-history.tsx#L51-L52apps/admin/src/app/(admin)/assets/[id]/valuation-history.tsx#L39-L40
🤖 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 `@apps/admin/src/app/`(admin)/assets/add-asset-dialog.tsx around lines 72 - 73,
Validate every parsed numeric value with Number.isFinite before negative-value
checks: update add-asset-dialog.tsx lines 72-73, asset-edit-client.tsx lines 57,
71, and 76, transaction-history.tsx lines 51-52 and 59, and
valuation-history.tsx lines 39-40. In add-asset-dialog.tsx, also preserve an
entered quantity of 0 instead of converting it to undefined at line 91.
Comment on lines
+60
to
+65
| {assets.map((asset) => ( | ||
| <TableRow | ||
| key={asset.id} | ||
| className="cursor-pointer hover:bg-muted/40 transition-colors border-b border-border" | ||
| onClick={() => router.push(`/assets/${asset.id}`)} | ||
| > |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Desktop rows are not reachable by keyboard.
The row navigation uses only onClick on TableRow. Keyboard users cannot open an asset from the desktop table. The mobile layout uses a button, so it works there.
Add a focusable link or keyboard handling. A Link in the name cell is the simplest option.
♿ Minimum fix on the row
<TableRow
key={asset.id}
+ tabIndex={0}
+ role="link"
+ aria-label={`Open ${asset.name}`}
className="cursor-pointer hover:bg-muted/40 transition-colors border-b border-border"
onClick={() => router.push(`/assets/${asset.id}`)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ router.push(`/assets/${asset.id}`);
+ }
+ }}
>📝 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.
Suggested change
| {assets.map((asset) => ( | |
| <TableRow | |
| key={asset.id} | |
| className="cursor-pointer hover:bg-muted/40 transition-colors border-b border-border" | |
| onClick={() => router.push(`/assets/${asset.id}`)} | |
| > | |
| {assets.map((asset) => ( | |
| <TableRow | |
| key={asset.id} | |
| tabIndex={0} | |
| role="link" | |
| aria-label={`Open ${asset.name}`} | |
| className="cursor-pointer hover:bg-muted/40 transition-colors border-b border-border" | |
| onClick={() => router.push(`/assets/${asset.id}`)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| router.push(`/assets/${asset.id}`); | |
| } | |
| }} | |
| > |
🤖 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 `@apps/admin/src/app/`(admin)/assets/assets-table.tsx around lines 60 - 65,
Make the desktop asset rows keyboard-accessible by adding a focusable navigation
control, preferably a Link in the asset name cell, targeting
`/assets/${asset.id}`. Update the TableRow rendering in the assets map while
preserving the existing click navigation and visual behavior.
Comment on lines
+20
to
+21
| const showDisposed = status === 'disposed'; | ||
| const kindFilter = kind === 'fixed_asset' || kind === 'investment' ? kind : undefined; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize status before you build the filter links.
showDisposed accepts only 'disposed', but the kind links pass the raw status value through. If a user opens /assets?status=foo, the page lists active assets and the kind links keep status=foo. Pass the normalized value instead.
🐛 Proposed fix
const showDisposed = status === 'disposed';
+ const statusFilter = showDisposed ? 'disposed' : undefined;Then use statusFilter in place of status at Lines 100, 108, and 116.
Also applies to: 100-116
🤖 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 `@apps/admin/src/app/`(admin)/assets/page.tsx around lines 20 - 21, Normalize
status once in the assets page before constructing filter links, using a
statusFilter value that preserves only the supported disposed state and
otherwise represents active assets. Update the kind-link query parameters around
the existing link construction to use statusFilter instead of the raw status,
including all occurrences near lines 100–116, while keeping showDisposed based
on the normalized value.
Comment on lines
+20
to
+24
| DO $$ BEGIN | ||
| IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'compliance_documents_client_id_clients_id_fk') THEN | ||
| ALTER TABLE "compliance_documents" ADD CONSTRAINT "compliance_documents_client_id_clients_id_fk" FOREIGN KEY ("client_id") REFERENCES "public"."clients"("id") ON DELETE cascade ON UPDATE no action; | ||
| END IF; | ||
| END $$;--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${DATABASE_URL:?Set DATABASE_URL to a disposable PostgreSQL database}"
psql "$DATABASE_URL" -v ON_ERROR_STOP=1 <<'SQL'
BEGIN;
CREATE TEMP TABLE migration_guard_collision (id integer);
ALTER TABLE migration_guard_collision
ADD CONSTRAINT billing_line_items_discount_amount_non_negative CHECK (id >= 0);
SELECT conname, conrelid::regclass
FROM pg_constraint
WHERE conname = 'billing_line_items_discount_amount_non_negative';
ROLLBACK;
SQLRepository: jchademwiri/pmg-hub
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/db/src/migrations/0041_compliance_leads_discounts.sql"
printf '%s\n' '--- migration ---'
cat -n "$file"
printf '%s\n' '--- related constraint names and table references ---'
rg -n -C 2 \
"compliance_documents_client_id_clients_id_fk|billing_line_items_discount_(percent_max|value_non_negative|amount_non_negative)|CREATE TABLE|ALTER TABLE" \
packages/db/src packages/db 2>/dev/null | head -n 240
printf '%s\n' '--- schema-qualified table definitions ---'
rg -n -C 3 \
'compliance_documents|billing_line_items|CREATE SCHEMA|search_path' \
packages/db/src packages/db 2>/dev/null | head -n 240
printf '%s\n' '--- all occurrences of the five constraint names ---'
rg -n \
"compliance_documents_client_id_clients_id_fk|billing_line_items_discount_percent_max|billing_line_items_discount_value_non_negative|billing_line_items_discount_amount_non_negative" \
. --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/nullRepository: jchademwiri/pmg-hub
Length of output: 50375
🌐 Web query:
PostgreSQL pg_constraint conname not necessarily unique across tables conrelid documentation
💡 Result:
In PostgreSQL, the conname column in the pg_constraint catalog is not necessarily unique across the entire database or even within a single namespace [1][2][3]. The PostgreSQL documentation explicitly states that constraint names are not required to be globally unique [1][2]. According to the PostgreSQL source code (specifically src/include/catalog/pg_constraint.h), this design is intentional for several reasons [4][5]: 1. Backwards Compatibility: It maintains compatibility with past PostgreSQL practices [4][6]. 2. Performance/Locking: It avoids the need to obtain a global lock to generate a unique name for nameless constraints [4][5]. 3. Namespace Association: While the connamespace column exists for SQL-spec compatibility, it is not used to enforce global uniqueness of constraint names [4][6]. Instead, PostgreSQL enforces uniqueness only at the level of the individual relation (table) or domain [4][5]. This is implemented via a unique index on the combination of conrelid (the OID of the table) or contypid (the OID of the domain) and conname [4][5][6]. Consequently, while two different tables can have constraints with the same name, a single table cannot have two constraints with the same name [4][7][5].
Citations:
- 1: https://www.postgresql.org/docs/current/catalog-pg-constraint.html
- 2: https://www.postgresql.org/docs/18/catalog-pg-constraint.html
- 3: https://www.postgresql.org/docs/15/catalog-pg-constraint.html
- 4: https://doxygen.postgresql.org/pg__constraint_8h_source.html
- 5: https://github.com/postgres/postgres/blob/10e4d8aa/src/include/catalog/pg_constraint.h
- 6: https://github.com/postgres/postgres/blob/92268b35d04c2de416279f187d12f264afa22614/src/include/catalog/pg_constraint.h
- 7: https://stackoverflow.com/questions/38718656/postgresql-constraint-name-is-not-necessarily-unique
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration schema and search-path context ---'
sed -n '1,125p' packages/db/src/migrate.ts
rg -n -C 3 'schema|search_path|pgTable|public' packages/db/src/schema packages/db/src/migrate.ts | head -n 180
printf '%s\n' '--- exact constraint declarations in repository ---'
rg -n \
'ADD CONSTRAINT "(compliance_documents_client_id_clients_id_fk|billing_line_items_discount_percent_max|billing_line_items_discount_value_non_negative|billing_line_items_discount_amount_non_negative)"|check\("(billing_line_items_discount_percent_max|billing_line_items_discount_value_non_negative|billing_line_items_discount_amount_non_negative)"' \
packages/db/src
printf '%s\n' '--- read-only parser check for duplicate constraint names across ALTER TABLE statements ---'
python3 - <<'PY'
import pathlib, re
names = {}
pattern = re.compile(
r'ALTER\s+TABLE\s+(?P<table>"[^"]+"|[A-Za-z_][\w.]*)\s+'
r'(?:ADD\s+CONSTRAINT|DROP\s+CONSTRAINT|RENAME\s+CONSTRAINT)\s+"(?P<name>[^"]+)"',
re.I,
)
for path in pathlib.Path("packages/db/src/migrations").glob("*.sql"):
text = path.read_text()
for match in pattern.finditer(text):
table, name = match.group("table"), match.group("name")
names.setdefault(name, []).append((str(path), table))
for name in (
"compliance_documents_client_id_clients_id_fk",
"billing_line_items_discount_percent_max",
"billing_line_items_discount_value_non_negative",
"billing_line_items_discount_amount_non_negative",
):
print(name, names.get(name, []))
duplicates = {name: sites for name, sites in names.items() if len({table for _, table in sites}) > 1}
print("cross-table duplicate names in repository:", duplicates)
PYRepository: jchademwiri/pmg-hub
Length of output: 18797
Scope each constraint lookup to its target table.
pg_constraint.conname is not unique across tables. A same-named constraint can cause these guards to skip the required constraint.
- Add
conrelid = 'compliance_documents'::regclassto the foreign-key lookup. - Add
conrelid = 'billing_line_items'::regclassto each discount-constraint lookup.
📍 Affects 1 file
packages/db/src/migrations/0041_compliance_leads_discounts.sql#L20-L24(this comment)packages/db/src/migrations/0041_compliance_leads_discounts.sql#L33-L47
🤖 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 `@packages/db/src/migrations/0041_compliance_leads_discounts.sql` around lines
20 - 24, Scope every pg_constraint lookup in migration 0041 by target table: add
conrelid = 'compliance_documents'::regclass to the compliance_documents
foreign-key guard at lines 20-24, and add conrelid =
'billing_line_items'::regclass to each discount-constraint guard at lines 33-47.
Comment on lines
+80
to
+85
| export async function reactivateAsset(id: string): Promise<void> { | ||
| await db | ||
| .update(assets) | ||
| .set({ status: "active", disposedAt: null, updatedAt: new Date() }) | ||
| .where(eq(assets.id, id)); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clear disposalNotes when you reactivate an asset.
reactivateAsset resets status and disposedAt but keeps disposalNotes. An active asset then carries notes from a previous disposal.
🐛 Proposed fix
- .set({ status: "active", disposedAt: null, updatedAt: new Date() })
+ .set({ status: "active", disposedAt: null, disposalNotes: null, updatedAt: new Date() })📝 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.
Suggested change
| export async function reactivateAsset(id: string): Promise<void> { | |
| await db | |
| .update(assets) | |
| .set({ status: "active", disposedAt: null, updatedAt: new Date() }) | |
| .where(eq(assets.id, id)); | |
| } | |
| export async function reactivateAsset(id: string): Promise<void> { | |
| await db | |
| .update(assets) | |
| .set({ status: "active", disposedAt: null, disposalNotes: null, updatedAt: new Date() }) | |
| .where(eq(assets.id, id)); | |
| } |
🤖 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 `@packages/db/src/queries/assets.ts` around lines 80 - 85, Update
reactivateAsset to also clear the asset’s disposalNotes when resetting status
and disposedAt, ensuring reactivated assets do not retain notes from a previous
disposal.
Comment on lines
+164
to
+177
| export async function addAssetValuation( | ||
| assetId: string, | ||
| data: { valuationDate: string; value: string; notes?: string | null }, | ||
| ): Promise<AssetValuationRow> { | ||
| const [inserted] = await db | ||
| .insert(assetValuations) | ||
| .values({ assetId, ...data }) | ||
| .returning(); | ||
| if (!inserted) throw new Error("Failed to record valuation."); | ||
|
|
||
| await syncCurrentValueFromLatestValuation(assetId); | ||
|
|
||
| return inserted; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Wrap the paired writes in a database transaction.
addAssetValuation, addAssetTransaction, and deleteAssetTransaction each perform two independent statements: a history write and an assets update. If the second statement fails, assets.currentValue or assets.quantity stays out of sync with the history rows. Concurrent calls on the same asset can also interleave.
Use db.transaction so each pair commits or rolls back together.
♻️ Example for addAssetTransaction
- const [inserted] = await db
- .insert(assetTransactions)
- .values({ assetId, ...data })
- .returning();
- if (!inserted) throw new Error("Failed to record transaction.");
-
- if (data.quantity != null) {
- const delta = data.type === "withdrawal" ? sql`-${data.quantity}::numeric` : sql`${data.quantity}::numeric`;
- await db
- .update(assets)
- .set({
- quantity: sql`greatest(coalesce(${assets.quantity}, 0) + ${delta}, 0)`,
- updatedAt: new Date(),
- })
- .where(eq(assets.id, assetId));
- }
-
- return inserted;
+ return db.transaction(async (tx) => {
+ const [inserted] = await tx
+ .insert(assetTransactions)
+ .values({ assetId, ...data })
+ .returning();
+ if (!inserted) throw new Error("Failed to record transaction.");
+
+ if (data.quantity != null) {
+ const delta = data.type === "withdrawal" ? sql`-${data.quantity}::numeric` : sql`${data.quantity}::numeric`;
+ await tx
+ .update(assets)
+ .set({
+ quantity: sql`greatest(coalesce(${assets.quantity}, 0) + ${delta}, 0)`,
+ updatedAt: new Date(),
+ })
+ .where(eq(assets.id, assetId));
+ }
+
+ return inserted;
+ });Also applies to: 223-251, 255-274
🤖 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 `@packages/db/src/queries/assets.ts` around lines 164 - 177, Wrap the paired
history and asset-update statements in db.transaction for addAssetValuation,
addAssetTransaction, and deleteAssetTransaction. Execute both operations through
the transaction-scoped database handle so they commit or roll back together and
concurrent updates on the same asset are serialized; preserve each function’s
existing return values and validation behavior.</code>
Comment on lines
+239
to
+248
| if (data.quantity != null) { | ||
| const delta = data.type === "withdrawal" ? sql`-${data.quantity}::numeric` : sql`${data.quantity}::numeric`; | ||
| await db | ||
| .update(assets) | ||
| .set({ | ||
| quantity: sql`greatest(coalesce(${assets.quantity}, 0) + ${delta}, 0)`, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(assets.id, assetId)); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The greatest(..., 0) clamp makes transaction deletion non-reversible.
A withdrawal larger than the held quantity is clamped to 0 instead of being rejected. deleteAssetTransaction then adds the full recorded quantity back. The asset quantity ends higher than before the withdrawal was recorded.
Reject a withdrawal that exceeds the held quantity, or store the applied delta so the reversal can subtract the same amount.
Also applies to: 263-273
🤖 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 `@packages/db/src/queries/assets.ts` around lines 239 - 248, Update the
quantity adjustment logic in deleteAssetTransaction and the corresponding
transaction update block so an over-withdrawal cannot be silently clamped by
greatest(..., 0). Reject withdrawals exceeding the held quantity, or persist and
reuse the actual applied delta so deletion reverses exactly the recorded change;
preserve normal deposits and valid withdrawals.
Comment on lines
+557
to
+559
| // Only invoices with a remaining balance - mirrors the status set used | ||
| // by the `outstanding` sum below (issued/overdue/partially_paid). | ||
| onlyOutstanding?: boolean; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter by positive balance, not only status.
onlyOutstanding is documented as selecting invoices with a remaining balance, but Line 593 checks only issued, overdue, and partially_paid. The division page calculates each balance from total - allocatedAmount. Therefore, an invoice with one of these statuses can still appear after its allocations reach or exceed the total. It will display as R0.00 or a negative amount and will also affect total.
Add the positive-balance predicate to the shared conditions array. Add regression tests for fully allocated and overallocated invoices.
Suggested fix
if (filters?.onlyOutstanding) {
- conditions.push(sql`${invoices.status} IN ('issued', 'overdue', 'partially_paid')`);
+ conditions.push(sql`
+ ${invoices.status} IN ('issued', 'overdue', 'partially_paid')
+ AND ${invoices.total}
+ - COALESCE((SELECT SUM(amount) FROM payment_allocations WHERE invoice_id = ${invoices.id}), 0)
+ - COALESCE((SELECT SUM(amount) FROM credit_applications WHERE invoice_id = ${invoices.id}), 0)
+ > 0
+ `);
}Also applies to: 592-594
🤖 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 `@packages/db/src/queries/billing.ts` around lines 557 - 559, Update the query
conditions used by the onlyOutstanding filter to require a strictly positive
remaining balance, calculated as invoice total minus allocatedAmount, in
addition to the existing outstanding-status predicate. Ensure this predicate is
added to the shared conditions array so both returned invoices and the
outstanding total exclude fully allocated and overallocated records, and add
regression coverage for both cases.
5 of 6 tasks
This branch was successfully deployed
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
project_scheduletable/enum rename,compliance_documents,leads.company_name, andbilling_line_itemsdiscount columns/checks that existed in the DB but were never migrated).bun db:generatenow runs cleanly with no interactive prompt and no stale diff.billing_itemsarchive-vs-delete pattern.bun db:generate.SidebarInsetmissingmin-w-0, so wide content forced the whole layout wider instead of scrolling internally) plus several tab/filter strips that hadoverflow-x-autorendered inert by the same issue (Leads, all Settings pages, Reports, a client detail page), and a tablet-only label-overlap bug on the dashboard's period selector.Notes for reviewers
IF NOT EXISTS/DO $$ ... IF NOT EXISTS ...) rather than rawdrizzle-kit generateoutput, because the generator kept mixing in an unrelated pre-existing migration backlog. Each is documented inline with why.document.body.scrollWidth === viewport widthplus screenshots at mobile and tablet widths across all major sections.Test plan
bun db:generateruns cleanly with no prompt and no diff on a fresh checkout🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Usability