feat: lease expiration notifications via cron + API#70
Conversation
Add automated lease expiration monitoring: - Daily cron trigger (9 AM UTC) checks for leases expiring within 30/60/90 days - Creates deduped tasks linked to each lease with urgency-based priority - Sends email (SendGrid) and SMS (Twilio) reminders when configured - New GET /api/leases/expiring?days=N endpoint for frontend consumption - ExpiringLeases dashboard widget with urgency badges - Wrangler cron triggers added to all 3 env blocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review Please evaluate:
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittyfinance | b723396 | Mar 26 2026, 11:44 PM |
📝 WalkthroughWalkthroughAdds end-to-end lease-expiration features: frontend widget and hook, new API route and storage queries, a scheduled worker that creates tasks and optionally sends email/SMS, and deployment cron entries to run the processor daily. Changes
Sequence DiagramsequenceDiagram
participant Cron as Scheduler (Cron)
participant Worker as Worker (server/worker.ts)
participant Storage as Database (SystemStorage)
participant Tasks as Task System (createTask)
participant Email as SendGrid
participant SMS as Twilio
Cron->>Worker: scheduled trigger (09:00 UTC)
Worker->>Storage: getExpiringLeases(window)
Storage-->>Worker: expiring lease rows
loop per window (0–30,31–60,61–90)
loop per lease
Worker->>Storage: getTasksByRelation('lease', leaseId)
Storage-->>Worker: existing tasks
alt no existing task for this window
Worker->>Tasks: create task (priority, metadata, dueDate)
Tasks-->>Worker: task created
Worker->>Email: send tenant email (if configured)
Email-->>Worker: success / error
Worker->>SMS: send tenant SMS (if configured)
SMS-->>Worker: success / error
end
end
end
Worker-->>Cron: log stats (checked, tasksCreated, emailsSent, smsSent, errors)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
PR Review: feat: lease expiration notifications via cron + APIOverall this is solid work — the cron integration, dedup logic, and component design are well-structured. A few issues need attention before merging. 🔴 Critical: Tenant isolation missing in
|
| Severity | Issue |
|---|---|
| 🔴 Critical | getExpiringLeases has no tenant filter — cross-tenant data leak in API |
| 🟡 Medium | 3 redundant DB queries + N×3 getTasksByRelation calls in cron |
| 🟡 Medium | useExpiringLeases dead import in Dashboard.tsx |
| 🟡 Medium | stats.checked overcounts leases |
| 🟠 Minor | Missing try/catch in route handler |
The tenant isolation issue is a blocker. Everything else can be addressed in follow-up or in this PR. The overall design — cron handler, dedup by task title, opt-in email/SMS, dashboard widget — is clean and matches the existing patterns well.
Reviewed by Claude Sonnet 4.6
There was a problem hiding this comment.
Pull request overview
Adds lease-expiration visibility and notifications to the system: a daily Cloudflare Scheduled handler generates lease-related tasks and optionally sends tenant reminders, plus a new API + dashboard widget for expiring leases.
Changes:
- Add a daily scheduled (cron) worker handler that scans for expiring leases and creates deduped tasks; optionally sends SendGrid/Twilio reminders.
- Add
GET /api/leases/expiring?days=Nand underlying storage queries to return expiring leases withdaysRemaining. - Add a dashboard widget to display expiring leases with urgency badges.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/worker.ts | Adds scheduled() handler to run lease expiration processing via cron. |
| server/lib/lease-expiration.ts | Implements the cron-driven lease expiration scan, task creation, and optional email/SMS notifications. |
| server/storage/system.ts | Adds expiring-lease query and a task lookup helper used for notification dedup. |
| server/routes/leases.ts | Introduces GET /api/leases/expiring endpoint. |
| server/app.ts | Mounts new lease routes and protects /api/leases with auth+tenant middleware. |
| deploy/system-wrangler.toml | Adds cron trigger configuration for scheduled execution across envs. |
| client/src/hooks/use-property.ts | Adds useExpiringLeases() hook + response typing. |
| client/src/components/property/ExpiringLeases.tsx | Adds dashboard card/table UI for expiring leases. |
| client/src/pages/Dashboard.tsx | Places the widget on the dashboard right column (tenant-selected). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (1)
server/lib/lease-expiration.ts (1)
78-98: LGTM - Resilient notification handling with proper error isolation.Good pattern: catching and logging individual notification failures allows processing to continue for remaining leases. This prevents a single email/SMS failure from blocking the entire batch.
One minor observation: errors are logged to console but there's no aggregation of failed notifications in the returned stats. Consider tracking
emailsFailedandsmsFailedif monitoring notification reliability is important.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/lease-expiration.ts` around lines 78 - 98, Add failure counters to the notification stats and increment them in the respective catch blocks: ensure the stats object (used by lease-expiration.ts) includes emailsFailed and smsFailed initialized to 0, then in the email catch block increment stats.emailsFailed and in the SMS catch block increment stats.smsFailed; use the existing symbols (sendgrid, twilio, EMAIL_TEMPLATES, SMS_TEMPLATES, window.smsTemplate, stats) so the try/catch blocks keep logging errors but also aggregate failures for reporting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/property/ExpiringLeases.tsx`:
- Around line 16-46: The component currently treats failed fetches the same as
an empty result; update ExpiringLeases to handle the error state from
useExpiringLeases: read isError and error (or equivalent) from the hook
alongside isLoading and leases, and when isError is true render a Card (same
header) showing an error message and optional retry hint or error.message to
distinguish API failures from "no leases"; ensure you reference
useExpiringLeases, leases, isLoading, isError and error when implementing this
change.
In `@server/lib/lease-expiration.ts`:
- Around line 46-55: The loop over NOTIFICATION_WINDOWS can create multiple
notifications for the same lease because getExpiringLeases(window.days) is
called per window and dedupe only checks taskTitle; fix by tracking
already-processed lease IDs within a single run: create a Set (e.g.,
processedLeaseIds) outside the for (const window of NOTIFICATION_WINDOWS) loop,
then before handling each { lease, unit, property } check if
processedLeaseIds.has(lease.id) and skip if true, and after creating a
notification/task add processedLeaseIds.add(lease.id); update code around
getExpiringLeases, the inner loop, and where tasks are created (references:
NOTIFICATION_WINDOWS, getExpiringLeases, storage.getTasksByRelation,
existingTasks, window.taskTitle, lease.id).
In `@server/routes/leases.ts`:
- Around line 9-13: Replace the manual parseInt/isNaN check for the days query
param with a Zod schema: create a schema (e.g., daysQuerySchema) using
z.coerce.number().int().min(1).max(365).optional().default(90) (or equivalent
preprocess) and call schema.parse or safeParse on c.req.query() to obtain the
validated days value instead of using parseInt; on validation failure return the
same 400 response with the Zod error details, and update references to the local
variable days to use the parsed value from the schema (look for uses of days and
c.req.query in this route handler).
In `@server/storage/system.ts`:
- Around line 354-374: getExpiringLeases currently returns leases across all
tenants; update its signature to accept a tenantId parameter and add a tenant
filter (e.g., eq(schema.leases.tenantId, tenantId)) to the where clause in
getExpiringLeases, then update the caller in server/routes/leases.ts to read
tenantId from the request context (c.get('tenantId')) and pass it into
storage.getExpiringLeases so queries are scoped to the requesting tenant; ensure
parameter types and any tests are updated accordingly.
In `@server/worker.ts`:
- Around line 13-17: Replace the .then()/.catch() promise chain on
processLeaseExpirations(env) with an async IIFE passed into ctx.waitUntil();
specifically, call ctx.waitUntil((async () => { try { const stats = await
processLeaseExpirations(env); console.log('Lease expiration check complete:',
JSON.stringify(stats)); } catch (err) { console.error('Lease expiration check
failed:', err); } })()); this uses async/await and preserves the original
logging while ensuring the task is awaited by the worker runtime (referencing
processLeaseExpirations and ctx in server/worker.ts).
---
Nitpick comments:
In `@server/lib/lease-expiration.ts`:
- Around line 78-98: Add failure counters to the notification stats and
increment them in the respective catch blocks: ensure the stats object (used by
lease-expiration.ts) includes emailsFailed and smsFailed initialized to 0, then
in the email catch block increment stats.emailsFailed and in the SMS catch block
increment stats.smsFailed; use the existing symbols (sendgrid, twilio,
EMAIL_TEMPLATES, SMS_TEMPLATES, window.smsTemplate, stats) so the try/catch
blocks keep logging errors but also aggregate failures for reporting.
🪄 Autofix (Beta)
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
Run ID: 0319498e-f11d-47c0-b70d-3b8e7c599f78
📒 Files selected for processing (9)
client/src/components/property/ExpiringLeases.tsxclient/src/hooks/use-property.tsclient/src/pages/Dashboard.tsxdeploy/system-wrangler.tomlserver/app.tsserver/lib/lease-expiration.tsserver/routes/leases.tsserver/storage/system.tsserver/worker.ts
- getExpiringLeases() now accepts optional tenantId for proper isolation - Lease route passes tenantId from middleware context - Action queue surfaces expiring leases as critical (<=30d) and warning (31-60d) items - PropertyDetailPanel shows lease end date column with urgency badge Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
🧩 Analysis chain🏁 Script executed: Length of output: 58018
Security Review: PR
|
| Area | Finding |
|---|---|
| Authentication | /api/leases and /api/leases/* are correctly included in protectedPrefixes in server/app.ts, applying auth + tenant + storage middleware. |
| Input validation | The days query param is validated as an integer between 1–365 before use. |
| SQL injection | Drizzle ORM parameterized queries throughout — no raw SQL interpolation. |
| Credential storage | All secrets (SENDGRID_API_KEY, TWILIO_ACCOUNT_SID, TWILIO_AUTH_TOKEN, etc.) are read exclusively from the env object (Cloudflare Worker secrets). No hardcoded credentials. |
| Supply chain | Zero new npm dependencies introduced. SendGridClient and TwilioClient are existing internal abstractions. |
| Cron attack surface | The scheduled handler is invoked by Cloudflare's scheduler only — there is no externally-triggerable HTTP surface. |
| Tenant scoping (API) | getExpiringLeases(days, tenantId) correctly passes the authenticated tenantId from context, scoping API results to the calling tenant. |
🔴 High Priority Issues
1. PII Logged in Error Messages (server/lib/lease-expiration.ts, Lines ~85–96)
Email addresses and phone numbers are interpolated directly into console.error log lines:
console.error(`Failed to send lease reminder email to ${lease.tenantEmail}:`, err);
console.error(`Failed to send lease reminder SMS to ${lease.tenantPhone}:`, err);Logs containing PII (email, phone) may be retained by Cloudflare's logging infrastructure (chittytrack tail consumer) and could constitute a GDPR/compliance violation.
Recommendation: Redact or omit PII from log messages:
console.error(`Failed to send lease reminder email (leaseId: ${lease.id}):`, err);
console.error(`Failed to send lease reminder SMS (leaseId: ${lease.id}):`, err);2. CPU Limit Too Low for Cron Workload (deploy/system-wrangler.toml, Line ~88)
[limits]
cpu_ms = 50The 50ms CPU limit applies globally, including to the scheduled handler. The cron iterates over 3 notification windows × N leases, each requiring multiple DB round-trips and potentially HTTP calls to SendGrid/Twilio. A property portfolio with even a modest number of expiring leases will hit this limit and be silently terminated mid-run, causing missed notifications with no observable error.
Recommendation: Raise the limit or move it to a non-cron-applicable block. Per Cloudflare's documentation, scheduled handlers are subject to CPU limits. A value of cpu_ms = 50 is appropriate only for very lightweight request handlers.
🟠 Medium Priority Issues
3. PII Exposed in API Response (server/routes/leases.ts, Lines ~17–30)
GET /api/leases/expiring returns tenantEmail and tenantPhone in the JSON payload:
tenantEmail: lease.tenantEmail,
tenantPhone: lease.tenantPhone,While this endpoint is authenticated, any property manager role can retrieve contact details for all tenants with expiring leases. Depending on your RBAC model, this may over-expose PII. The frontend ExpiringLeases component does not display email or phone — they are fetched but unused.
Recommendation: Either omit these fields from the response unless required by consumers, or implement field-level permissions.
4. getTasksByRelation Lacks Tenant Isolation (server/storage/system.ts)
async getTasksByRelation(relatedTo: string, relatedId: string) {
return this.db.select().from(schema.tasks)
.where(and(
eq(schema.tasks.relatedTo, relatedTo),
eq(schema.tasks.relatedId, relatedId),
));
}Unlike every other task query in SystemStorage (e.g., getTasks(tenantId)), this method has no tenant filter. The cron's deduplication logic relies on this method. While UUID collision between tenants is highly improbable, this is architecturally inconsistent and could behave incorrectly in multi-tenant edge cases.
Recommendation: Add an optional tenantId parameter:
async getTasksByRelation(relatedTo: string, relatedId: string, tenantId?: string) {
const conditions = [
eq(schema.tasks.relatedTo, relatedTo),
eq(schema.tasks.relatedId, relatedId),
];
if (tenantId) conditions.push(eq(schema.tasks.tenantId, tenantId));
return this.db.select().from(schema.tasks).where(and(...conditions));
}Then pass property.tenantId in the cron's dedup call.
5. Staging Environment Shares Production KV Namespace ID (deploy/system-wrangler.toml)
[env.staging.kv_namespaces]
binding = "FINANCE_KV"
id = "517b63be4d7144c197b5bdf851f12041" # ← same as productionThe staging environment uses the same KV namespace ID as production. If staging's DATABASE_URL secret also points to the production database, the staging cron (firing daily at 9 AM UTC) will send real notifications to real tenants from the staging worker.
Recommendation: Confirm that DATABASE_URL in the staging environment points to a staging database, and use a dedicated staging KV namespace ID.
🟡 Low Priority Issues
6. Cloudflare Account ID Committed to Repository (deploy/system-wrangler.toml, Line 18)
account_id = "0bc21e3a5a9de1a4cc843be9c3e98121"Cloudflare does not treat account_id as a secret, but committing it to a (potentially public) repository makes it trivially discoverable. Combined with a stolen API token, it reduces the number of steps an attacker needs.
Recommendation: Move account_id to a CI/CD environment variable (CLOUDFLARE_ACCOUNT_ID) and reference it via wrangler deploy --account-id $CLOUDFLARE_ACCOUNT_ID rather than embedding it in the TOML.
7. No Retry or Dead-Letter Mechanism for Failed Notifications
Email and SMS failures are caught and logged, but there is no retry. If SendGrid or Twilio is temporarily unavailable during the cron window, the task is still created (preventing re-notification on the next run), but the tenant never receives their email/SMS.
Recommendation: Consider tracking notification delivery state in the task's metadata field, or use a separate retry queue. At minimum, document this known gap.
8. Cross-Tenant Cron Scope (Informational)
processLeaseExpirations calls storage.getExpiringLeases(window.days) without a tenantId, so it reads active leases across all tenants in the database. This appears intentional for a system-level cron, but it is not documented. Tasks are created with the correct property.tenantId, so there is no cross-tenant write risk — only read.
Recommendation: Add a comment clarifying this is intentional system-wide behavior, distinguishing it from the per-tenant API endpoint.
Breaking API Changes
None. The new GET /api/leases/expiring endpoint is purely additive. The getExpiringLeases storage method signature change (tenantId?: string added as an optional second param) is backward-compatible.
Summary
| Category | Result |
|---|---|
| Security implications | |
| Credential exposure | ✅ No hardcoded secrets; minor: account_id in TOML |
| Supply chain | ✅ No new dependencies |
| Breaking changes | ✅ None |
| Cron reliability |
🧠 Learnings used
Learnt from: CR
Repo: chittyapps/chittyfinance PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-07T14:48:32.602Z
Learning: Keep PRs small and focused; include summary, rationale, and screenshots for UI changes; link related issues; document env/DB changes and run `npm run db:push` when schema updates
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
PR Review: feat: lease expiration notifications via cron + APIOverall, this is a well-structured implementation of the Phase 4 lease expiration feature. The integration points (cron handler, storage methods, API route, frontend widget, action queue) all fit cleanly into existing patterns. A few issues need attention before merge. 🐛 Bug — Email/SMS sent 3× for near-expiring leases (must fix)
The dedup check ( Fix: process only the most specific (smallest) window that matches each lease. The simplest approach is to query once for 90-day expiries, sort ascending by // In processLeaseExpirations — fetch once, classify in memory
const allExpiring = await storage.getExpiringLeases(90);
for (const { lease, unit, property } of allExpiring) {
const days = Math.ceil((new Date(lease.endDate).getTime() - Date.now()) / 86_400_000);
const window = NOTIFICATION_WINDOWS.find((w) => days <= w.days); // first match = smallest window
if (!window) continue;
// ... rest of logic unchanged
}🐛 N+1 queries in cron deduplication (should fix)
Pre-fetch all const allLeaseTasks = await storage.getTasksByRelation('lease');
// then filter in memory: allLeaseTasks.filter(t => t.relatedId === lease.id)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/components/property/PropertyDetailPanel.tsx (1)
281-290: Consider extracting the shared urgency badge logic to reduce duplication.The badge variant calculation (
days <= 30→ destructive,days <= 60→ default, else secondary) is duplicated here and inExpiringLeases.tsx(theurgencyBadgefunction at lines 9-14). Extracting this to a shared utility would ensure consistent thresholds across components.Additionally, the magic number
86_400_000(milliseconds per day) could be a named constant.♻️ Suggested refactor
Create a shared utility (e.g.,
client/src/lib/lease-utils.ts):const MS_PER_DAY = 24 * 60 * 60 * 1000; export function getDaysRemaining(endDate: string | Date): number { return Math.ceil((new Date(endDate).getTime() - Date.now()) / MS_PER_DAY); } export function getLeaseUrgencyVariant(daysRemaining: number): 'destructive' | 'default' | 'secondary' { if (daysRemaining <= 30) return 'destructive'; if (daysRemaining <= 60) return 'default'; return 'secondary'; }Then use it in both components:
-{lease ? (() => { - const days = Math.ceil((new Date(lease.endDate).getTime() - Date.now()) / 86_400_000); - return ( - <Badge variant={days <= 30 ? 'destructive' : days <= 60 ? 'default' : 'secondary'}> - {new Date(lease.endDate).toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: '2-digit' })} - </Badge> - ); -})() : '\u2014'} +{lease ? (() => { + const days = getDaysRemaining(lease.endDate); + return ( + <Badge variant={getLeaseUrgencyVariant(days)}> + {new Date(lease.endDate).toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: '2-digit' })} + </Badge> + ); +})() : '\u2014'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/property/PropertyDetailPanel.tsx` around lines 281 - 290, Extract the duplicated badge/urgency logic into a shared utility (e.g., create client/src/lib/lease-utils.ts) that defines a named MS_PER_DAY constant and exports two helpers: getDaysRemaining(endDate: string | Date) which computes Math.ceil((new Date(endDate).getTime() - Date.now()) / MS_PER_DAY), and getLeaseUrgencyVariant(daysRemaining: number) which returns 'destructive' for days <= 30, 'default' for days <= 60, otherwise 'secondary'; then replace the inline calculation in PropertyDetailPanel (the anonymous function rendering the Badge) and the existing urgencyBadge in ExpiringLeases.tsx to call getDaysRemaining and getLeaseUrgencyVariant so both components use the single source of truth for thresholds and the MS_PER_DAY constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/storage/system.ts`:
- Around line 378-388: getTasksByRelation currently queries schema.tasks without
tenant isolation which allows cross-tenant matches; change its signature to
require tenantId and add a where clause filtering schema.tasks.tenantId =
tenantId, then update the caller in lease-expiration (the call that passes
property.tenantId) to supply that tenantId so dedupe uses tenant-scoped results;
likewise audit and fix getTask (the getTask(id) method) to include tenantId
filtering and update any API routes that call getTask to pass/verify tenantId to
prevent cross-tenant access.
---
Nitpick comments:
In `@client/src/components/property/PropertyDetailPanel.tsx`:
- Around line 281-290: Extract the duplicated badge/urgency logic into a shared
utility (e.g., create client/src/lib/lease-utils.ts) that defines a named
MS_PER_DAY constant and exports two helpers: getDaysRemaining(endDate: string |
Date) which computes Math.ceil((new Date(endDate).getTime() - Date.now()) /
MS_PER_DAY), and getLeaseUrgencyVariant(daysRemaining: number) which returns
'destructive' for days <= 30, 'default' for days <= 60, otherwise 'secondary';
then replace the inline calculation in PropertyDetailPanel (the anonymous
function rendering the Badge) and the existing urgencyBadge in
ExpiringLeases.tsx to call getDaysRemaining and getLeaseUrgencyVariant so both
components use the single source of truth for thresholds and the MS_PER_DAY
constant.
🪄 Autofix (Beta)
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
Run ID: 78f8f0ad-f696-48e3-8334-65a1700e702f
📒 Files selected for processing (4)
client/src/components/property/PropertyDetailPanel.tsxclient/src/hooks/use-action-queue.tsserver/routes/leases.tsserver/storage/system.ts
… removal - Fix duplicate notifications: use non-overlapping date ranges (0-30, 31-60, 61-90) so each lease matches exactly one window - Fix silent cron failures: await processLeaseExpirations directly instead of ctx.waitUntil so Cloudflare detects cron failures - Add failure counters (emailsFailed, smsFailed, errors[]) to stats so notification failures are visible, not silently swallowed - Wrap per-lease processing in try-catch so one DB error doesn't kill the entire batch - Add DATABASE_URL guard before Neon connection attempt - Log when SendGrid/Twilio are not configured (disambiguates "0 sent") - Remove tenant PII (email, phone) from /api/leases/expiring response — frontend doesn't use them - Add try-catch to API route to prevent raw DB errors leaking to clients - Simplifications: extract shared Header component, replace IIFE with named LeaseEndBadge, consolidate action queue tiers, extract MS_PER_DAY constant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review Please evaluate:
|
|
The security review for PR 🔒 Security Implications
🔑 Credential Exposure Risk
📦 Dependency Supply Chain
🔁 Breaking API Changes
⚙️ Reliability Concern (Bonus)
🧠 Learnings used✅ Actions performedReview triggered.
|
Code Review — PR #70: Lease Expiration NotificationsOverall this is well-structured work. The non-overlapping window design, dedup-by-title pattern, and graceful degradation when SendGrid/Twilio aren't configured are all solid choices. A few things to address before merging: Bugs1. Unused import in
- import { usePortfolioSummary, useExpiringLeases } from '@/hooks/use-property';
+ import { usePortfolioSummary } from '@/hooks/use-property';2. Unhandled rejection in scheduled handler (
async scheduled(controller: ScheduledController, env: Env, ctx: ExecutionContext) {
try {
const stats = await processLeaseExpirations(env);
console.log('[cron:lease-expiration] complete:', JSON.stringify(stats));
if (stats.errors.length > 0) {
console.error(`[cron:lease-expiration] ${stats.errors.length} failures during processing`);
}
} catch (err) {
console.error('[cron:lease-expiration] fatal error:', err);
}
},Code Quality3.
4. N+1 query pattern in For every expiring lease, there's a separate for (const { lease, unit, property } of expiring) {
const existingTasks = await storage.getTasksByRelation('lease', lease.id); // N queriesFor small portfolios this is fine, but it's worth noting. A single query fetching all const leaseIds = expiring.map(({ lease }) => lease.id);
const allExistingTasks = await storage.getTasksByRelationBatch('lease', leaseIds);
// then filter locally per lease5. While not strictly required (the scheduled handler awaits completion), the idiomatic CF Workers pattern is to wrap long-running work in async scheduled(controller, env, ctx) {
ctx.waitUntil(processLeaseExpirations(env).then(stats => {
console.log('[cron:lease-expiration] complete:', JSON.stringify(stats));
}).catch(err => {
console.error('[cron:lease-expiration] fatal error:', err);
}));
},Minor Notes
Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/lease-expiration.ts`:
- Around line 95-113: The current catch blocks in lease-expiration build
messages that include PII (lease.tenantEmail and lease.tenantPhone) and push
them to stats.errors and console.error; change both places (the email catch and
the sms catch around twilio.sendSms and SMS_TEMPLATES) to redact or omit
tenantEmail/tenantPhone from the msg: compose messages that reference lease.id
and the error text only (e.g., "Email/SMS for lease <lease.id>: <err.message>")
and update stats.errors.push(...) and console.error(...) to use the scrubbed
message so no raw PII is logged or stored.
🪄 Autofix (Beta)
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
Run ID: fd316276-caca-4d03-98f6-ee8723c6176f
📒 Files selected for processing (8)
client/src/components/property/ExpiringLeases.tsxclient/src/components/property/PropertyDetailPanel.tsxclient/src/hooks/use-action-queue.tsclient/src/hooks/use-property.tsserver/lib/lease-expiration.tsserver/routes/leases.tsserver/storage/system.tsserver/worker.ts
✅ Files skipped from review due to trivial changes (2)
- client/src/components/property/PropertyDetailPanel.tsx
- client/src/components/property/ExpiringLeases.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- client/src/hooks/use-property.ts
- server/worker.ts
- server/routes/leases.ts
- client/src/hooks/use-action-queue.ts
- server/storage/system.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
relatedTo='lease'tasks with window-specific titles, skips if already notifiedlease_reminder) and Twilio (lease_reminder_30/60/90) templates when configuredGET /api/leases/expiring?days=Nreturns leases with tenant, property, unit, anddaysRemainingExpiringLeasescomponent shows upcoming expirations with urgency badges (red <=30d, default <=60d, muted <=90d)[triggers] crons = ["0 9 * * *"]added to top-level + all 3 env blocksTest plan
npm run checkpasses (TypeScript clean)npm run buildsucceedsGET /api/leases/expiring?days=90returns correct data shapewrangler dev --test-scheduledGenerated with Claude Code
Summary by CodeRabbit