feat: Track D API hygiene (pre-iOS) - #121
Conversation
Normalize error envelopes and Clerk auth options, audit sync and membership mutations, soft-delete recurring rules, and share currency validation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📜 Recent review details🔇 Additional comments (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds soft deletion for recurring transactions, filters deleted rules from reads and processing, expands audit logging for mutations, standardizes Clerk authentication and JSON errors, and reuses shared currency validation for account and debt schemas. ChangesPlatform data and API behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
🤖 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/web/server/api/audit.ts`:
- Line 127: Update the recurring rule authorization query around
isNull(recurringTransactions.deletedAt) to allow tombstoned records while
retaining the existing household and visibility checks. Use the established
tombstone-aware authorization pattern for recurring_transactions so deleted
rules remain available for audit log reads instead of producing NOT_FOUND.
In `@apps/web/server/api/invites.ts`:
- Around line 178-205: Remove plaintext email fields from the audit snapshot
newValues objects at apps/web/server/api/invites.ts lines 178-205 and 587-610,
and apps/web/server/api/members.ts lines 592-606 and 711-725. Preserve stable
IDs and required role fields while retaining emails only where an approved
retention and access-control policy explicitly requires them.
In `@apps/web/server/api/sync.ts`:
- Around line 286-314: The toggle update at apps/web/server/api/sync.ts lines
286-314 must add an isNull(groceryItems.deletedAt) predicate and reject the
mutation when no active row is returned, preventing audit updates after soft
deletion. The tag replacement flow at apps/web/server/api/sync.ts lines 373-423
must condition its writes on the grocery item remaining active and reject the
mutation if deletion occurred before replacement.
In `@apps/web/server/lib/errors.ts`:
- Around line 35-37: Update the response body construction in jsonError so extra
is spread before the required error and code fields, ensuring callers cannot
overwrite either field while preserving the existing HTTP status mapping.
In `@apps/web/server/lib/recurring-processor.ts`:
- Line 190: Make occurrence creation atomic with the deletion check by
revalidating the recurring rule’s deletedAt state within the write-time
transaction or conditional insert, before inserting the transaction. Update the
processor flow around candidateRules and advanceRecurringRuleIfCurrent so a rule
deleted after selection cannot create an occurrence, while preserving
advancement behavior for non-deleted rules.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3109485c-d3af-42a1-b573-d713c82b4ae2
📒 Files selected for processing (24)
apps/web/app/routes/budget.recurring.tsxapps/web/server/api/accounts.tsapps/web/server/api/audit.tsapps/web/server/api/budgets.tsapps/web/server/api/calendar.tsapps/web/server/api/debts.tsapps/web/server/api/invites.tsapps/web/server/api/members.tsapps/web/server/api/push.tsapps/web/server/api/recurring.tsapps/web/server/api/route.test.tsapps/web/server/api/route.tsapps/web/server/api/sync.tsapps/web/server/lib/clerk-auth-options.tsapps/web/server/lib/dashboard-data.tsapps/web/server/lib/errors.tsapps/web/server/lib/financial-categories.tsapps/web/server/lib/recurring-processor.tsapps/web/server/middleware/app-context.tsapps/web/worker.test.tsapps/web/worker.tspackages/db/migrations/0019_recurring_soft_delete.sqlpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema/recurring.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-06-21T23:05:02.739Z
Learnt from: j-cadena-g
Repo: j-cadena-g/amigo PR: 64
File: packages/db/migrations/0013_financial_categories.sql:21-31
Timestamp: 2026-06-21T23:05:02.739Z
Learning: In this codebase, tenant/household isolation for DB writes is enforced at the application layer using `scopeToHousehold()` (not via DB-level composite foreign keys or household-bound CHECK constraints). During review of SQL migrations, don’t require DB-layer household isolation constraints for every change as long as the affected write paths are covered by `scopeToHousehold()`. Treat additional DB-layer constraints as defense-in-depth that should be handled via the separately tracked follow-ups (e.g., for `budget_category_mappings`) rather than being mandatory for each migration PR.
Applied to files:
packages/db/migrations/0019_recurring_soft_delete.sql
🔇 Additional comments (24)
apps/web/server/api/accounts.ts (2)
2-3: LGTM!
25-27: LGTM!apps/web/server/api/debts.ts (1)
23-25: LGTM!apps/web/server/lib/clerk-auth-options.ts (1)
1-18: LGTM!apps/web/server/lib/errors.ts (1)
1-18: LGTM!apps/web/server/middleware/app-context.ts (1)
5-5: LGTM!Also applies to: 25-28
apps/web/server/api/route.ts (1)
11-12: LGTM!Also applies to: 56-56, 73-79, 108-135
apps/web/worker.ts (1)
12-13: LGTM!Also applies to: 110-124, 139-147
apps/web/server/api/push.ts (1)
3-3: LGTM!Also applies to: 108-110, 141-143
apps/web/server/api/route.test.ts (1)
128-128: LGTM!Also applies to: 266-266, 284-287
apps/web/worker.test.ts (1)
83-83: LGTM!Also applies to: 102-102
packages/db/migrations/0019_recurring_soft_delete.sql (1)
1-3: LGTM!apps/web/server/api/budgets.ts (1)
91-92: LGTM!apps/web/server/lib/dashboard-data.ts (1)
216-216: LGTM!Also applies to: 395-395
apps/web/server/lib/financial-categories.ts (1)
121-122: LGTM!apps/web/server/api/members.ts (1)
43-43: LGTM!Also applies to: 162-177, 375-395, 460-465
apps/web/server/api/invites.ts (1)
27-27: LGTM!Also applies to: 290-327
apps/web/server/api/sync.ts (1)
16-16: LGTM!Also applies to: 56-56, 196-196, 252-272, 318-354, 426-429
packages/db/migrations/meta/_journal.json (1)
137-143: LGTM!packages/db/src/schema/recurring.ts (1)
64-72: LGTM!apps/web/server/api/recurring.ts (1)
5-5: LGTM!Also applies to: 85-86, 188-189, 297-298, 324-325, 345-352, 385-386
apps/web/app/routes/budget.recurring.tsx (1)
12-12: LGTM!Also applies to: 35-36
apps/web/server/api/calendar.ts (1)
162-168: LGTM!apps/web/server/lib/recurring-processor.ts (1)
6-6: LGTM!Also applies to: 164-164
Allow audit of soft-deleted recurring rules, drop emails from membership audit snapshots, harden sync/processor races, and lock jsonError envelope fields.
1 participant
Summary
{ error, code }and share Clerk auth option shapes across middleware, clerk routes, and WebSocket upgrades0019) and filter tombstones from live queries/processor pathszCurrencyCodein accounts and debts schemasTest plan
pnpm exec vitest run --config vitest.config.ts server/api/route.test.ts worker.test.tspnpm exec vitest run --config vitest.workers.config.ts server/api/invites.integration.test.ts server/api/members.integration.test.ts0019_recurring_soft_delete.sqlapplies cleanly