Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 1 · needs revision
3 open — 3 🟡 Low (see inline)
577c376 to
9699d22
Compare
Revision — Cycle 1fixed R1-L1, R1-L2, R1-L3 · 9699d22 |
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 2 · needs revision
1 open — 1 🔴 Critical (see inline)
manager_digest shipped in #547 but nothing ever wrote it. Renaming it now and adding manager_weekly_digest is data-preserving since the column is still unused. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
previousOrgDay, currentOrgWeekStart and previousOrgWeek do their arithmetic on the org calendar date, never a resolved instant, so a DST transition inside the shifted range can't shift the day count. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getDailyDigestRecipients/getWeeklyDigestRecipients compute recipients from EmailLog alone (no new column) — a manager already digested inside the current calendar day/week is dropped and counted as skipped, so a repeat cron fire is a no-op. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two static routes, not one route with a mode param — each names and runs exactly the one digest its path names, so it stays a falsifiable allowlist line rather than a job runner. Both reject any request whose Authorization header isn't the CRON_SECRET bearer token before touching the database. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Appends both cron routes to CLAUDE.md's allowlist per #569's shape, and updates ENGINEERING/PERMISSIONS/WORKFLOWS for the new swallow site, the machine-authenticated route, and the two digest workflows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The weekly email no longer tallies "new applications this week" — a manager gets it only when they have applications still short of a terminal status, with a status-count box per status (color-coded like the in-app badges) instead of a text recap. previousOrgWeek has no remaining caller and is removed along with it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Weekly digest redesign (operator feedback)Per direct feedback from the operator (not a formal review cycle):
|
Replaces the previous-org-calendar-day window with a gap-safe one anchored to each manager's own last digest (24h fallback for a first-ever digest). A missed or delayed cron fire can no longer drop an application, and in steady daily operation the window is still always ~24h. The email now names the exact since-instant rather than a calendar day, since the window can span more than a day after a gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Daily digest windowing redesign (operator feedback)Per direct feedback, the daily digest no longer windows to the previous org calendar day. It now windows since each manager's own last digest (any status, success or failure — a failed run still isn't retried, same philosophy as before), falling back to
|
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 4 · needs revision
2 open — 1 🟠 Medium, 1 🟡 Low (see inline)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b-at-neu
left a comment
There was a problem hiding this comment.
Code Review — Cycle 5 · approved
0 open
Closes #549
Summary
accepted/rejected/withdrawn/draft) across the manager's positions, as a status-count box per status (colored to match the in-app badge palette), plus their currently-open positions as context. Sent only when that total is greater than zero. This is a design change from the originally-agreed plan (which specified a "new this week" total plus a full status breakdown including terminal decisions) — see the follow-up comment on Manager Daily Digest #549 recording why and what shipped instead.EmailLogrow already existing in the current org week; the daily window is itself the idempotency mechanism (see above).EmailTemplateKey.manager_digest(shipped in Email Foundation And Send Log #547, never written) tomanager_daily_digestand addsmanager_weekly_digest.CLAUDE.md's API-route allowlist (rewritten by Ingest Resend Delivery Events #569) with the bounding clause that they are not a general job runner.Changes
prisma/schema.prisma,prisma/migrations/20260911222818_manager_digest_cadences/— enum rename + add, hand-writtenALTER TYPE ... RENAME VALUE/ADD VALUE(data-preserving).lib/dates.ts—previousOrgDay,currentOrgWeekStart(org calendar-date arithmetic, DST-safe);previousOrgWeekwas added then removed once the weekly digest stopped needing a week-bounded query.lib/constants.ts— renamed/addedEMAIL_TEMPLATE_VALUES/EMAIL_TEMPLATE_LABELSentries,MANAGER_DIGEST_SEND_SPACING_MS,DAILY_DIGEST_LOOKBACK_MS, rewordedcomplained("recipient" not "applicant").lib/utils.ts—getEmailLogDescription's two bounce sentences reworded to "recipient".lib/types.ts—ManagerDigestPosition,DailyDigestRecipient(since: Date),WeeklyDigestStatusCount,WeeklyDigestRecipient(asOfDay: string, unresolved-onlystatusCounts).prisma/data/digests.ts— new;getDailyDigestRecipientsfetches raw application rows and buckets per manager by their ownsincecutoff (can't pre-aggregate until each manager's cutoff is known);getWeeklyDigestRecipientstallies onlyUNRESOLVED_APPLICATION_STATUSES, no time bound. Both scoped byPosition.managersonly (never the admin-short-circuitingbuildReviewablePositionWhere).lib/email/templates.ts—managerDailyDigestEmail(names the exact since-instant),managerWeeklyDigestEmail(table-based colored stat-box grid per status, matching the in-app badge palette),MANAGER_EMAIL_FOOTER.lib/email/manager-digests.ts— new;dispatchDailyManagerDigests/dispatchWeeklyManagerDigests, the second named swallow site (per-recipienttry/catch, spaced byMANAGER_DIGEST_SEND_SPACING_MS).lib/cron.ts—rejectUnauthorizedCron, the bearer-secret check both routes call before any DB access.app/api/cron/manager-daily-digest/route.ts,app/api/cron/manager-weekly-digest/route.ts— new.vercel.json— the twocronsentries.CLAUDE.md,docs/ENGINEERING.md,docs/PERMISSIONS.md,docs/WORKFLOWS.md,README.md,.env.example— allowlist, logging swallow site, route-access note,XC-10workflow entry +PM-8/AD-12clauses,CRON_SECRETdocs.tests/unit/dates.test.ts,tests/unit/email-templates.test.ts,tests/db/manager-digests.test.ts— new coverage;tests/db/email-log-queries.test.ts,tests/unit/utils.test.ts— updated for the renamed enum value and reworded copy.Testing plan
CRON_SECRETin the Vercel preview env and redeploy (also needsRESEND_API_KEY/RESEND_FROM_EMAIL, already set).curl -i <preview>/api/cron/manager-daily-digestwith no header, then a wrong one →401 {"error":"Unauthorized"}both times; repeat for…/manager-weekly-digest;/emailsshows no new rows.Mwith two open positions, submit two applications to the first and one to the second.curl -H "Authorization: Bearer $CRON_SECRET" <preview>/api/cron/manager-daily-digest→{"sent":1,...};Mgets one email, subject3 new applications across 2 positions, both positions listed with the right counts, body naming the exact since-instant./manage/applications?positionId=…with the new submissions on top.{"sent":0,...,"skipped":1}and no second email.Nwith no new activity gets nothing on the daily run.Nhaving some applications stillapplied/reviewing(any age) and noneaccepted/rejected/withdrawn/draft,curl … /api/cron/manager-weekly-digest→ one email with a stat box per unresolved status and the open-positions list.skipped: 1; re-run the daily one → unaffected by the weekly row.Nonly terminal-status applications (accepted/rejected) → the weekly run sends nothing toN./emailsas an admin →Manager daily digest/Manager weekly digestrows addressed toM/N; both filterable by template; neither appears in any application's Email history.M, submit another application, re-run both → no digest forM.Automated checks
npm run prettier:check— passnpm run eslint:check— passnpm run tsc:check— passnpm run test— 781 passed (42 files), run against a local throwaway Postgres container since the worktree's default port 5432 was occupied by an unrelated containernpm run build— pass; both cron routes appear as dynamic (ƒ) routesNotes
EmailLogrow (any status), so a run that errors just gets picked up in full by the next one for the daily digest, or waits a full week for the weekly one; thefailedrow on/emailsis the record either way.submittedAt, notApplicationStatusEvent, is the "received" source for the daily digest — refreshed on awithdrawn → appliedresubmission, excludes since-withdrawn rows, unaffected by a reviewer overriding a status back toapplied.