Repository navigation
fix: apply data integrity constraints and harden API routes - #9
Conversation
- Add unique indexes for bills and meter readings to prevent duplicate entries - Improve R2 file upload pipeline to ensure clean rollback on database insertion failure - Implement strict type validation in recalculation logic - Expand Vitest coverage across all major API route boundaries - Fix CI workflow configurations to properly run coverage and builds
📝 WalkthroughWalkthroughThe PR adds API integration tests, improves upload rollback handling, validates recalculation inputs before mutation, adds a data-integrity migration, configures Vitest coverage, and strengthens local and CI validation. ChangesApplication reliability and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 12
🧹 Nitpick comments (2)
src/api/routes/notifications.test.ts (1)
86-142: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest cross-user notification mutations.
Seed a second user and notification for these mutation tests. Attempt to mark that notification as read as
"user-1"and assert rejection and unchanged state. Also verify thatPOST /read-allleaves the second user's notification unread.The current tests do not protect the
userIdauthorization boundary for write operations.Based on learnings: Provide unit tests for all route guards, library logic, and controllers.
🤖 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/api/routes/notifications.test.ts` around lines 86 - 142, Extend the PATCH /{id}/read and POST /read-all tests to seed a notification belonging to a second user, then verify user-1 cannot mark it read: assert the request is rejected and its readAt remains null. Also assert POST /read-all marks only user-1’s notifications as read while the second user’s notification stays unread.Source: Learnings
src/api/lib/property-auth.test.ts (1)
117-141: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an inactive-tenancy denial test.
The test covers only
status: "active". Seed an inactive tenancy for"tenant-id"and assert thatrequirePropertyAccessreturnsnull. This protects the active-status authorization condition.Based on learnings: Provide unit tests for all route guards, library logic, and controllers.
🤖 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/api/lib/property-auth.test.ts` around lines 117 - 141, Add a test alongside the existing requirePropertyAccess cases that seeds a tenancy for "tenant-id" with an inactive status, invokes requirePropertyAccess, and asserts it returns null. Keep the existing active-tenant and owner coverage unchanged.Source: Learnings
🤖 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 @.github/workflows/ci.yml:
- Around line 60-66: Update the CI test workflow around the “Run tests” step to
invoke the existing pnpm test:coverage script instead of pnpm test --run,
preserving NODE_ENV: test so Vitest runs coverage and enforces the configured
thresholds.
In @.husky/pre-push:
- Around line 4-7: Update the pre-push documentation in CONTRIBUTING.md to list
all checks executed by the hook: astro check, pnpm lint, and pnpm test --run.
Keep the existing contributor instructions and formatting consistent.
In `@package.json`:
- Line 14: Align the Vitest package and coverage provider versions in the
package manifest to the same release, ensuring `@vitest/coverage-v8` matches
vitest. Then regenerate pnpm-lock.yaml so the resolved packages and peer
dependencies use that single version consistently.
In `@src/api/lib/recalculation.ts`:
- Around line 45-66: The validation loop over periodsToProcess must stop before
a confirmed downstream period that is not the recalculation target, matching the
execution loop boundary. Add this break before parsing oneOffCharges and
rateOverride, while allowing the target confirmed period to continue through
validation.
- Around line 45-66: Extend the pre-validation loop over periodsToProcess to
parse and Zod-validate oneOffCharges as an array and rateOverride as an object
containing consumptionRate, before recalculation or any db.delete(bills)
operation. Keep the existing malformed-JSON errors and add validation errors
with period context, using the existing schemas if available.
In `@src/api/routes/custom-charges.test.ts`:
- Around line 75-118: Update the owner POST test in the custom-charges request
suite to expect the route’s 201 response, then verify the created charge is
persisted. Extend the owner DELETE test to confirm the charge is removed, and
capture database state before and after unauthorized POST and DELETE requests to
assert those operations leave persistence unchanged.
In `@src/api/routes/dashboard.test.ts`:
- Line 10: Update the dashboard tenant test to use a mutable currentUser, create
or seed a non-owner tenant user, and assign that user before the
/dashboard/tenant request in the affected test flow. Keep the owner setup for
owner-specific assertions, but ensure the tenant endpoint is invoked with the
tenant user’s identity.
In `@src/api/routes/uploads.test.ts`:
- Around line 270-271: Update the concurrent-upload status assertion to require
exactly one successful response and one rate-limited response: replace the
partial contains check with an equality assertion against the sorted statuses
[200, 429].
In `@src/api/routes/uploads.ts`:
- Around line 225-249: Update objectKey construction in the upload handler to
append crypto.randomUUID() to the timestamp-based filename before r2.put(),
while preserving the existing property, period, user, and extension segments.
- Around line 128-138: Update uploadPhotoRoute to define and apply a single Zod
schema for the multipart fields photo, periodId, propertyId, purpose, and
editRequestId before any auth query or downstream processing. Replace the direct
formData.get casts with validated parsed values, return a 400 response for
schema failures, and preserve the existing rate-limit rollback behavior on those
failures.
In `@src/db/migrations/0002_data_integrity_fixes.sql`:
- Around line 8-14: Update the duplicate-cleanup migration around the
billing_periods DELETE to first identify each canonical newest period, then
re-parent or explicitly delete related bills, meter_readings, edit_requests, and
bill_photos before removing duplicate periods. Preserve one newest period per
(property_id, period_month), and validate the migration with a fixture
containing duplicates and every dependent record type.
- Around line 40-48: The migration currently creates only daily-counter unique
indexes; add the missing unique index creation statements for billing periods,
meter readings, bills, and active tenancies using the names and column
definitions declared by the generated schema snapshot. If equivalent indexes
already exist in an earlier migration, regenerate or update the snapshot
instead, ensuring the migrated database enforces all declared uniqueness
constraints.
---
Nitpick comments:
In `@src/api/lib/property-auth.test.ts`:
- Around line 117-141: Add a test alongside the existing requirePropertyAccess
cases that seeds a tenancy for "tenant-id" with an inactive status, invokes
requirePropertyAccess, and asserts it returns null. Keep the existing
active-tenant and owner coverage unchanged.
In `@src/api/routes/notifications.test.ts`:
- Around line 86-142: Extend the PATCH /{id}/read and POST /read-all tests to
seed a notification belonging to a second user, then verify user-1 cannot mark
it read: assert the request is rejected and its readAt remains null. Also assert
POST /read-all marks only user-1’s notifications as read while the second user’s
notification stays unread.
🪄 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: 04e75b0d-7486-4f60-86ba-595e109d767e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.github/workflows/ci.yml.husky/pre-pushCONTRIBUTING.mdeslint.config.mjspackage.jsonsrc/api/AGENTS.mdsrc/api/lib/notifications.test.tssrc/api/lib/property-auth.test.tssrc/api/lib/recalculation.test.tssrc/api/lib/recalculation.tssrc/api/routes/bills.test.tssrc/api/routes/custom-charges.test.tssrc/api/routes/dashboard.test.tssrc/api/routes/export.test.tssrc/api/routes/notifications.test.tssrc/api/routes/tenancies.test.tssrc/api/routes/uploads.test.tssrc/api/routes/uploads.tssrc/api/routes/users.test.tssrc/db/AGENTS.mdsrc/db/migrations/0002_data_integrity_fixes.sqlsrc/db/migrations/meta/0002_snapshot.jsonsrc/db/migrations/meta/_journal.jsontsconfig.jsonvitest.config.mts
| - name: Run tests | ||
| run: pnpm test --run | ||
| env: | ||
| # Vitest needs this to stay in test mode | ||
| NODE_ENV: test | ||
|
|
||
| - name: Build (catch Astro + Wrangler errors) | ||
| run: pnpm build:ci |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## workflow excerpt"
if [ -f .github/workflows/ci.yml ]; then
nl -ba .github/workflows/ci.yml | sed -n '1,140p'
else
echo ".github/workflows/ci.yml not found"
fi
echo
echo "## vitest config files"
fd -a 'vitest\.config\.|vitest\.|\.config\.' . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "## package scripts"
if [ -f package.json ]; then
python3 - <<'PY'
import json
p=json.load(open('package.json'))
print(json.dumps(p.get('scripts', {}), indent=2))
PY
fi
echo
echo "## coverage references"
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'coverage|test:coverage|pnpm test|vitest|thresholds|lines|functions|branches|statements' .github package.json .config 2>/dev/null || trueRepository: VaibhavDaveDev/Meterly
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## workflow excerpt"
if [ -f .github/workflows/ci.yml ]; then
awk '{printf "%6d\t%s\n", NR, $0}' .github/workflows/ci.yml | sed -n '1,140p'
else
echo ".github/workflows/ci.yml not found"
fi
echo
echo "## candidate config/package files"
python3 - <<'PY'
import os
files=[]
for root, dirs, filenames in os.walk('.'):
dirs[:] = [d for d in dirs if d not in {'.git','node_modules','dist','build','.github'}]
for f in filenames:
if f in {'package.json','vitest.config.mts','vitest.config.ts'} or f.startswith('vitest.config.'):
files.append(os.path.join(root,f))
for f in files:
print(f"--- {f}")
with open(f, encoding='utf-8', errors='replace') as fh:
for i, line in enumerate(fh, 1):
print(f"{i:4}\t{line.rstrip()}")
PY
echo
echo "## coverage references"
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'coverage|test:coverage|pnpm test|vitest|thresholds|lines|functions|branches|statements' .github package.json .config 2>/dev/null || trueRepository: VaibhavDaveDev/Meterly
Length of output: 9565
Run coverage in CI.
The test script is vitest and coverage is only enabled by pnpm test:coverage. Add a CI coverage step or replace .github/workflows/ci.yml:61 with pnpm test:coverage so Vitest enforces the configured thresholds.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 32-66: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml around lines 60 - 66, Update the CI test workflow
around the “Run tests” step to invoke the existing pnpm test:coverage script
instead of pnpm test --run, preserving NODE_ENV: test so Vitest runs coverage
and enforces the configured thresholds.
| echo "⏳ Running lint before push..." | ||
| pnpm lint | ||
| echo "⏳ Running tests before push..." | ||
| pnpm test --run |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the pre-push documentation.
The hook now runs lint and tests, but CONTRIBUTING.md Line 90 documents only astro check. List all three pre-push checks.
Based on the updated hook and the existing contributor instructions.
🤖 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 @.husky/pre-push around lines 4 - 7, Update the pre-push documentation in
CONTRIBUTING.md to list all checks executed by the hook: astro check, pnpm lint,
and pnpm test --run. Keep the existing contributor instructions and formatting
consistent.
| "deploy": "astro build && node -e \"require('fs').rmSync('dist/server/wrangler.json',{force:true}); require('fs').rmSync('.wrangler/deploy/config.json',{force:true})\" && wrangler pages deploy --project-name meterly", | ||
| "lint": "eslint .", | ||
| "test": "vitest", | ||
| "test:coverage": "vitest run --coverage", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env sh
set -eu
pnpm view `@vitest/coverage-v8`@4.1.10 peerDependenciesRepository: VaibhavDaveDev/Meterly
Length of output: 1087
🏁 Script executed:
#!/usr/bin/env sh
set -eu
echo "package scripts:"
node -e 'const p=require("./package.json"); console.log(JSON.stringify(p.scripts,null,2))'
echo
echo "dependencies around vitest:"
node -e 'const p=require("./package.json"); for (const [k,v] of Object.entries({...p.dependencies,...p.devDependencies,...p.peerDependencies}).entries()) if (/vitest|coverage/.test(k)) console.log(k, v)'
echo
echo "lockfile vitest entries:"
python3 - <<'PY'
from pathlib import Path
p=Path("pnpm-lock.yaml")
if p.exists():
text=p.read_text(errors="replace").splitlines()
for i,line in enumerate(text):
if "vitest@" in line or "`@vitest/coverage-v8`@" in line or ("vitest:" in line and "packages:" not in line):
start=max(0,i-3); end=min(len(text),i+8)
print(f"--- around line {i+1} ---")
for j in range(start,end):
print(f"{j+1}: {text[j]}")
PY
echo
echo "npm metadata for versions:"
npm view vitest@4.1.9 version peerDependencies --json
npm view `@vitest/coverage-v8`@4.1.9 version peerDependencies --json
npm view vitest@4.1.10 version peerDependencies --json
npm view `@vitest/coverage-v8`@4.1.10 version peerDependencies --jsonRepository: VaibhavDaveDev/Meterly
Length of output: 15286
Align all Vitest packages to one version.
vitest@4.1.9 requires @vitest/coverage-v8@4.1.9; the lockfile installs @vitest/coverage-v8@4.1.10(vitest@4.1.9) while its peer is vitest: 4.1.10. Set Vitest and the coverage provider to the same version, then refresh pnpm-lock.yaml.
🤖 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 `@package.json` at line 14, Align the Vitest package and coverage provider
versions in the package manifest to the same release, ensuring
`@vitest/coverage-v8` matches vitest. Then regenerate pnpm-lock.yaml so the
resolved packages and peer dependencies use that single version consistently.
| for (const p of periodsToProcess) { | ||
| if (p.oneOffCharges) { | ||
| try { | ||
| JSON.parse(p.oneOffCharges); | ||
| } catch (e) { | ||
| throw new Error( | ||
| `[recalculation] Malformed oneOffCharges JSON on period ${p.id}: ${String(e)}`, | ||
| { cause: e } | ||
| ); | ||
| } | ||
| } | ||
| if (p.rateOverride) { | ||
| try { | ||
| JSON.parse(p.rateOverride); | ||
| } catch (e) { | ||
| throw new Error( | ||
| `[recalculation] Malformed rateOverride JSON on period ${p.id}: ${String(e)}`, | ||
| { cause: e } | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop pre-validation at the confirmed-period boundary.
The execution loop stops before a confirmed downstream period. This loop still parses that period. If a later confirmed period contains malformed JSON, recalculation of valid earlier periods now fails even though that confirmed period would not be processed.
Break this validation loop when p is a non-target confirmed period. Keep validation for the target period, because explicit recalculation can process it.
🤖 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/api/lib/recalculation.ts` around lines 45 - 66, The validation loop over
periodsToProcess must stop before a confirmed downstream period that is not the
recalculation target, matching the execution loop boundary. Add this break
before parsing oneOffCharges and rateOverride, while allowing the target
confirmed period to continue through validation.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'recalculation\.ts|package\.json|zod' . | sed 's#^\./##'
echo "== git diff stat =="
git diff --stat || true
echo "== outline recalculation =="
ast-grep outline src/api/lib/recalculation.ts --view expanded || true
echo "== read recalculation relevant =="
wc -l src/api/lib/recalculation.ts
cat -n src/api/lib/recalculation.ts | sed -n '1,180p'
echo "== search oneOffCharges rateOverride occurrences =="
rg -n "oneOffCharges|rateOverride|override\.consumptionRate|consumptionRate|periodsToProcess" src/api src -g '*.ts' | head -200Repository: VaibhavDaveDev/Meterly
Length of output: 24383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== recalculation parser usage =="
cat -n src/api/lib/recalculation.ts | sed -n '160,225p'
echo "== billing-periods zod and rateOverride =="
cat -n src/api/routes/billing-periods.ts | sed | sed -n '1,150p'
echo "rateOverride occurrences billing-periods:"
cat -n src/api/routes/billing-periods.ts | sed -n '150,230p'
echo "== package zod availability =="
node - <<'JS'
try {
const path = require.resolve('zod');
console.log('zod module path:', path);
const z = require('zod');
console.log('zod version:', z?.version || 'unknown');
const schema = z.array(z.object({ name: z.string(), amount: z.number(), chargedToTenant: z.boolean() }));
for (const value of [null, [], {}, '']) {
try {
console.log(value, JSON.stringify(value), 'parse=>', JSON.stringify(schema.parse(value)));
} catch (e) {
console.log(value, JSON.stringify(value), 'error=>', e?.name + ': ' + e.message.split("\n")[0]);
}
}
} catch (e) {
console.log('zod unavailable/error:', e.message);
}
JS
echo "== read-only behavioral probe of parsed shapes =="
node - <<'JS'
const inputs = {
oneOffCharges: [null, [], {}, ''],
rateOverride: [null, {}, [], '', '{ "value": 1 }']
};
for (const [field, shapes] of Object.entries(inputs)) {
for (const shape of shapes) {
let parsed;
try {
parsed = shape === '' ? null : JSON.parse(shape);
} catch (e) {
console.log('JSON parse failed', field, JSON.stringify(shape), e.message);
continue;
}
let ok = true, err;
if (field === 'oneOffCharges' && parsed) {
try { [...parsed]; } catch (e) { ok = false; err = e.toString(); }
} else if (field === 'rateOverride' && parsed) {
try { const x = parsed.consumptionRate; } catch (e) { ok = false; err = e.toString(); }
}
console.log(`${field}:${shape}; parsed=${JSON.stringify(parsed)}; compatible=${ok}; error=${ok ? '' : err}`);
}
}
JSRepository: VaibhavDaveDev/Meterly
Length of output: 4454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== billing-periods zod and rateOverride =="
cat -n src/api/routes/billing-periods.ts | sed -n '1,160p'
echo "rateOverride occurrences billing-periods:"
cat -n src/api/routes/billing-periods.ts | sed -n '150,210p'
echo "== package zod availability =="
node - <<'JS'
try {
const path = require.resolve('zod');
console.log('zod module path:', path);
const z = require('zod');
console.log('zod version:', z?.version || 'unknown');
const schema = z.array(z.object({ name: z.string(), amount: z.number(), chargedToTenant: z.boolean() }));
for (const value of [null, [], {}, '']) {
try {
console.log(value, JSON.stringify(value), 'parse=>', JSON.stringify(schema.parse(value)));
} catch (e) {
console.log(value, JSON.stringify(value), 'error=>', e?.name + ': ' + e.message.split("\n")[0]);
}
}
} catch (e) {
console.log('zod unavailable/error:', e.message);
}
JS
echo "== read-only behavioral probe of parsed shapes =="
node - <<'JS'
const inputs = {
oneOffCharges: [null, [], {}, ''],
rateOverride: [null, {}, [], '', '{ "value": 1 }']
};
for (const [field, shapes] of Object.entries(inputs)) {
for (const shape of shapes) {
let parsed;
try {
parsed = shape === '' ? null : JSON.parse(shape);
} catch (e) {
console.log('JSON parse failed', field, JSON.stringify(shape), e.message);
continue;
}
let ok = true, err;
if (field === 'oneOffCharges' && parsed) {
try { [...parsed]; } catch (e) { ok = false; err = e.toString(); }
} else if (field === 'rateOverride' && parsed) {
try { const x = parsed.consumptionRate; } catch (e) { ok = false; err = e.toString(); }
}
console.log(`${field}:${shape}; parsed=${JSON.stringify(parsed)}; compatible=${ok}; error=${ok ? '' : err}`);
}
}
JSRepository: VaibhavDaveDev/Meterly
Length of output: 10102
Validate period JSON values before recalculation starts.
oneOffCharges must be an array and rateOverride must contain consumptionRate. Move Zod validation of these parsed values into the pre-validation loop so failures occur before any db.delete(bills) runs.
🤖 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/api/lib/recalculation.ts` around lines 45 - 66, Extend the pre-validation
loop over periodsToProcess to parse and Zod-validate oneOffCharges as an array
and rateOverride as an object containing consumptionRate, before recalculation
or any db.delete(bills) operation. Keep the existing malformed-JSON errors and
add validation errors with period context, using the existing schemas if
available.
Source: Coding guidelines
| it("POST / returns 201 for the owner", async () => { | ||
| const res = await app.request( | ||
| `/custom-charges/${propertyId}/charges`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ name: "Internet", amount: 30 }), | ||
| }, | ||
| mockEnv as never | ||
| ); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
|
|
||
| it("POST / returns 403 for unauthorized user", async () => { | ||
| currentUser = { id: "stranger-id" }; | ||
| const res = await app.request( | ||
| `/custom-charges/${propertyId}/charges`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ name: "Internet", amount: 30 }), | ||
| }, | ||
| mockEnv as never | ||
| ); | ||
| expect(res.status).toBe(403); | ||
| }); | ||
|
|
||
| it("DELETE /{id} returns 200 for the owner", async () => { | ||
| const res = await app.request( | ||
| `/custom-charges/charges/${chargeId}`, | ||
| { method: "DELETE" }, | ||
| mockEnv as never | ||
| ); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
|
|
||
| it("DELETE /{id} returns 403 for unauthorized user", async () => { | ||
| currentUser = { id: "stranger-id" }; | ||
| const res = await app.request( | ||
| `/custom-charges/charges/${chargeId}`, | ||
| { method: "DELETE" }, | ||
| mockEnv as never | ||
| ); | ||
| expect(res.status).toBe(403); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Verify the write contract and persisted state.
The POST test name specifies HTTP 201, but Line 85 accepts HTTP 200. Align the test name and expectation with the route contract.
Assert that an owner POST creates the charge and an owner DELETE removes it. Also assert that unauthorized requests leave the database unchanged. Status-only checks do not detect mutation before authorization failure.
🤖 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/api/routes/custom-charges.test.ts` around lines 75 - 118, Update the
owner POST test in the custom-charges request suite to expect the route’s 201
response, then verify the created charge is persisted. Extend the owner DELETE
test to confirm the charge is removed, and capture database state before and
after unauthorized POST and DELETE requests to assert those operations leave
persistence unchanged.
| const statuses = [res1.status, res2.status].sort(); | ||
| expect(statuses).toContain(429); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete concurrent-upload result.
toContain(429) passes if both requests fail or if the other request returns 500. The expected result is exactly one successful reservation and one rate-limited reservation.
Replace this assertion with expect(statuses).toEqual([200, 429]).
🤖 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/api/routes/uploads.test.ts` around lines 270 - 271, Update the
concurrent-upload status assertion to require exactly one successful response
and one rate-limited response: replace the partial contains check with an
equality assertion against the sorted statuses [200, 429].
Source: Learnings
| const formData = await c.req.formData(); | ||
| const photo = formData.get("photo") as File | null; | ||
| const periodId = formData.get("periodId") as string | null; | ||
| const propertyId = formData.get("propertyId") as string | null; | ||
| const purpose = formData.get("purpose") as | ||
| | "import_meter" | ||
| | "export_meter" | ||
| | "solar_meter" | ||
| | "bill_document" | ||
| | null; | ||
| const editRequestId = formData.get("editRequestId") as string | null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)uploads\.ts$|openapi-schemas|api/routes' || true
echo
echo "uploads outline:"
ast-grep outline src/api/routes/uploads.ts --view compact || true
echo
echo "Relevant uploads sections:"
wc -l src/api/routes/uploads.ts
sed -n '1,220p' src/api/routes/uploads.ts
echo
echo "Search uploadPhotoRoute/route definitions:"
rg -n "uploadPhotoRoute|router\.openapi|FormData|formData\.get|propertyId|periodId|purpose|editRequestId" src/api/routes/uploads.tsRepository: VaibhavDaveDev/Meterly
Length of output: 9487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/api/routes/uploads.ts")
text = p.read_text()
lines = text.splitlines()
for idx,line in enumerate(lines, start=1):
if "const formData = await c.req.formData" in line:
print(f"\nFile: {p}")
for start in range(max(1, idx-2), min(len(lines), idx+12) + 1):
print(f"{start:4}: {lines[start-1]}")
PYRepository: VaibhavDaveDev/Meterly
Length of output: 847
Validate each multipart field with Zod.
formData.get() does not enforce runtime type; a non-string periodId or propertyId can run through the auth query before validation and still produce a 500 response instead of a 400. Add one multipart request schema to uploadPhotoRoute covering photo, periodId, propertyId, purpose, and editRequestId, then keep the rate-limit rollback for schema failures.
🤖 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/api/routes/uploads.ts` around lines 128 - 138, Update uploadPhotoRoute to
define and apply a single Zod schema for the multipart fields photo, periodId,
propertyId, purpose, and editRequestId before any auth query or downstream
processing. Replace the direct formData.get casts with validated parsed values,
return a 400 response for schema failures, and preserve the existing rate-limit
rollback behavior on those failures.
Source: Coding guidelines
| // --- Store in R2 --- | ||
| // Key: propertyId/periodId/userId/timestamp.webp | ||
| const timestamp = Date.now(); | ||
| const MIME_TO_EXT: Record<string, string> = { | ||
| "image/webp": "webp", | ||
| "image/jpeg": "jpg", | ||
| "image/jpg": "jpg", | ||
| "image/png": "png", | ||
| "image/heic": "heic", | ||
| "image/heif": "heif", | ||
| "application/pdf": "pdf", | ||
| }; | ||
| const ext = MIME_TO_EXT[photo.type] ?? "bin"; | ||
| objectKey = `${propertyId}/${periodId}/${user.id}/${timestamp}.${ext}`; | ||
|
|
||
| await r2.put(objectKey, await photo.arrayBuffer(), { | ||
| httpMetadata: { contentType: photo.type }, | ||
| customMetadata: { | ||
| uploadedBy: user.id, | ||
| periodId, | ||
| propertyId, | ||
| purpose, | ||
| uploadedAt: new Date().toISOString(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make each R2 object key collision-resistant.
Date.now() has millisecond precision. Concurrent uploads for the same property, period, user, and extension can generate the same key. The second insert then conflicts on bill_photos.object_key, and the catch block deletes the shared R2 object. The successful database row then points to a missing object.
Append crypto.randomUUID() to objectKey before r2.put().
🤖 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/api/routes/uploads.ts` around lines 225 - 249, Update objectKey
construction in the upload handler to append crypto.randomUUID() to the
timestamp-based filename before r2.put(), while preserving the existing
property, period, user, and extension segments.
| -- Keep the newest billing period per (property_id, period_month). | ||
| DELETE FROM billing_periods | ||
| WHERE rowid NOT IN ( | ||
| SELECT MAX(rowid) | ||
| FROM billing_periods | ||
| GROUP BY property_id, period_month | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Delete or re-parent dependent records before deleting duplicate billing periods.
billing_periods has NO ACTION foreign-key dependents. These include bills, meter_readings, edit_requests, and bill_photos. The parent delete can abort the migration when a duplicate period has related records.
Select the canonical period first. Then migrate or explicitly remove its dependent records before deleting the duplicate period. Validate this migration against a database fixture that contains duplicate periods and all dependent record types.
Based on learnings, “Validate database migrations before deployment.”
🤖 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/db/migrations/0002_data_integrity_fixes.sql` around lines 8 - 14, Update
the duplicate-cleanup migration around the billing_periods DELETE to first
identify each canonical newest period, then re-parent or explicitly delete
related bills, meter_readings, edit_requests, and bill_photos before removing
duplicate periods. Preserve one newest period per (property_id, period_month),
and validate the migration with a fixture containing duplicates and every
dependent record type.
Source: Learnings
| -- 2. Named unique indexes for daily-counter tables. | ||
| -- Drizzle schema declares uniqueIndex("upload_daily_count_user_date_unique") | ||
| -- and uniqueIndex("reading_daily_count_user_date_unique"). | ||
| CREATE UNIQUE INDEX IF NOT EXISTS upload_daily_count_user_date_unique | ||
| ON upload_daily_count (user_id, date_key); | ||
| --> statement-breakpoint | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS reading_daily_count_user_date_unique | ||
| ON reading_daily_count (user_id, date_key); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Create the bill and meter-reading unique indexes in this migration.
The migration removes duplicate billing_periods, bills, and active tenancies, but it only creates indexes for daily counters. The generated snapshot declares unique indexes for billing periods, meter readings, bills, and active tenancies. Without matching CREATE UNIQUE INDEX statements, a migrated database does not enforce the intended bill and meter-reading invariants.
Add the missing index creation statements, or regenerate the snapshot if those indexes already exist in an earlier migration.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 43-44: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
[warning] 47-48: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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/db/migrations/0002_data_integrity_fixes.sql` around lines 40 - 48, The
migration currently creates only daily-counter unique indexes; add the missing
unique index creation statements for billing periods, meter readings, bills, and
active tenancies using the names and column definitions declared by the
generated schema snapshot. If equivalent indexes already exist in an earlier
migration, regenerate or update the snapshot instead, ensuring the migrated
database enforces all declared uniqueness constraints.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Documentation