feat(spend-alerts): optimize sweep, fix panel UX, scope org recipients - #6670
Conversation
…nd-alerts-optimize-ux-scopes-0d5a/s2)
…ucket retention job (kwf spend-alerts-optimize-ux-scopes-0d5a/s3)
…nd name the scope and the measures (kwf spend-alerts-optimize-ux-scopes-0d5a/s5)
…nly (kwf spend-alerts-optimize-ux-scopes-0d5a/s4)
…elta, phase timings (kwf spend-alerts-optimize-ux-scopes-0d5a/s1)
…measures, and check for dead space (kwf spend-alerts-optimize-ux-scopes-0d5a/s6)
…ot move the dashboard (kwf spend-alerts-optimize-ux-scopes-0d5a/c1)
…u (kwf spend-alerts-optimize-ux-scopes-0d5a/ux1)
…s (kwf spend-alerts-optimize-ux-scopes-0d5a/ux2)
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit is test-only hygiene (organization cleanup moved from Files Reviewed (1 file)
Prior Findings Re-checked
Previous Review Summaries (3 snapshots, latest commit 598179b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 598179b)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit resolves the prior open finding — a removed organization creator no longer receives organization spend alerts — by requiring a current membership row for the creator branch and re-resolving organization recipients at drain time; no new issues were found in the changed code. Files Reviewed (4 files)
Previous review (commit 11c20cb)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commits resolve the prior migration-number collision (the rollup covering index is regenerated as migration Files Reviewed (5 files)
Previous review (commit f41e511)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe spend-alert sweep, retention, recipient, and UX changes are sound; the merge-blocking risk is that the new migration reuses number Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Files Reviewed (26 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
The branch carried 0258_spend_alerts_rollup_covering_index.sql with idx 258, which collides with main's own 0258_github_connection_role. Merge origin/main and regenerate the migration with the drizzle CLI. - Drop the branch's 0258 migration file and its snapshot. - Take main's migration folder verbatim (0254-0259). - Regenerate as 0260_spend_alerts_rollup_covering_index.sql (journal idx 260). - Restore the COMMIT;/BEGIN; boundary the transactional migrator needs for CREATE INDEX CONCURRENTLY, which spend-alerts-rollup-index-migration.test.ts asserts. The regenerated SQL matches the previous file byte for byte. Guards: migration-journal.test.ts and spend-alerts-rollup-index-migration.test.ts both pass.
… window The run's cap defers the re-derived remainder, and the rotation only finishes inside the LOOKBACK_HOURS window while the remainder fits under MAX_REMAINDER_FLOOR x the window's ticks. Above that a deferred scope stops being re-derived before its turn, so a crossing it would have alerted on is never decided. Derive the window's tick count and the rotation target from LOOKBACK_HOURS and the cron interval instead of the bare literals, state the bound the guarantee actually has, and log a warning with the shortfall when the rotation cannot meet it.
Assert the per-tick phase log reports remainderTicks against rollupWindowTicks, and that a remainder needing exactly the whole window reports 36 of 36 without warning.
…rating main has since appended its own 0260-0262 entries, so the branch's generated SQL, snapshot and journal entry are removed and will be regenerated at the next free index from the merged schema.
pnpm drizzle-kit generate --name spend_alerts_rollup_covering_index on the merged tree emits the concurrent btree over (created_at, kilo_user_id, organization_id, cost) and nothing else; the COMMIT;/BEGIN; migrator boundaries are appended per packages/db/AGENTS.md.
…mize-ux-scopes-0d5a
A removed organization creator stayed eligible for the organization's spend alerts. The creator branch read `created_by_kilo_user_id`, which is history and is never cleared, and treated it as an owner even when the creator no longer had a membership, which is the only thing `ensureOrganizationAccess` reads as access. Require a membership row of the organization for the creator branch, so the role stays unrestricted (a creator whose membership says `member` still receives the alert) but a removed creator does not. Re-check the organization's current contacts when the drain sends a queued alert as well: the outbox row carries the recipients resolved at enqueue time and may be sent an hour later, so the snapshot is now narrowed to the contacts that still hold access. A personal scope is unchanged.
The organization cleanup of this suite ran in an afterAll hook. The jest worker teardown closes the database pool in its own afterAll, and that hook runs first. The delete therefore hit a closed pool: "Failed query: delete from organizations where id in ($1, $2)", cause "Cannot use a pool after calling end on the pool". All 14 tests passed, but the suite failed on that hook. Move the cleanup into the existing afterEach, which runs before the pool closes. No dependent rows need removal first: the membership rows have no foreign key to organizations, and the hook already deletes the suite's own delivery rows.
…mize-ux-scopes-0d5a
…mize-ux-scopes-0d5a
Changelog for users
Account: <name>orOrganization: <name>.billing_managerrole; an admin without that role no longer receives them.Changelog for maintainers
now.hour_startat 720 h.*/5cron; a daily0 1 * * *cron prunesspend_alert_hourlybuckets older than 30 days in 10,000-row batches.microdollar_usage (created_at, kilo_user_id, organization_id, cost)withCREATE INDEX CONCURRENTLY, so the 1.6B-row table takes no blocking lock.billing_managermembers; who may edit settings is unchanged, and the query stays scoped to the one organization.en.json; the email already rendersscope_nameandkind_label, and the push lock-screen body stays content-free.E2E proof
[e14] ux-check: On the web panel the line under the 'Spend alerts' title reads 'Account: ' on the personal view and 'Organization: ' on the organization view. — android emulator-5554, platform android: as the org owner (e14-login-owner.log 'signed in as e2e-org-owner-spend-alerts-optimize-ux-scopes-0d5a@example.com') the personal spend-alerts screen (Profile > Preferences > Spend alerts) shows the scope row label 'Account' with value 'E2E Org Owner' (e14-personal.log/e14-personal.png) and the organization screen (Profile > account selector 'Acme Corp' > Manage organization > Spend alerts) shows 'Organization' with 'Acme Corp' plus 'Threshold crossing'/'This scope's rolling spend over the chosen window crosses your limit' and 'Hourly spike'/'This…
[e13] ux-check: hold the loading skeleton then let the settings resolve — the 'Enable spend alerts' row and the rule cards do not move — android emulator-5554: nextjs was stalled so the settings query stayed pending and the screen mounted fresh from Profile > Preferences > Spend alerts; the skeleton digest shows the placeholder blocks (e13-layout.log: [55,342][1025,573] scope group, [55,628][1025,775] enable row), then nextjs was recovered and the same mounted screen resolved to the loaded form ('Account'/'E2E Org Owner', 'Enable spend alerts', 'Threshold crossing', 'Hourly spike'). Screenshots e13-skeleton.png and e13-loaded-enabled.png are captured for the visual reviewer, which owns the pixel judgement of the vertical…
[e8] needs:fault: mobile spend-alerts load error on the device — with the spendAlerts.get request failed the screen shows its error copy and a Retry, and the screen stays put — android emulator-5554 (platform android): state settings reported STATE HIT (e8-state.log) and nextjs was killed ('fault.sh: nextjs killed (port 7100 refuses; pids 787117 )', e8-fault-down.log); the scene reached the Spend alerts screen and logged 'SCENE e8 OK' with 'android.widget.TextView Couldn't load spend alerts. tappable [345,1235][736,1281]' and 'android.widget.Button Retry tappable [461,1318][619,1434]' under the screen header 'android.view.View Spend alerts tappable [111,102][1044,167]' with the tab bar still present, so the screen stayed put (capture e8.png for the visual reviewer)…
[e7] mobile spend-alerts screen on the device: the empty state, then enabling the alerts shows both rule cards — each with its push toggle — whose descriptions say what is measured and a scope line naming… — SCENE e7 OK (e7-scene.log): the empty state showed only 'Enable spend alerts'; after enabling the digest showed the scope row 'Account'/'e2e-mobile-spend-alerts-optimize-ux-scopes-0d5a-android', 'Threshold crossing' + "This scope's rolling spend over the chosen window crosses your limit", 'Hourly spike' + "This scope's hourly rate is far above its own p95 baseline" and the 'Threshold crossing and Push'/'Hourly spike and Push' toggles, Save showed 'Spend alert settings saved' and the run ended on 'Spend alerts are off. Turn them on to alert this scope's billing contacts.'; DB after (e7.log)…
[e14] ux-check: On the web panel the line under the 'Spend alerts' title reads 'Account: ' on the personal view and 'Organization: ' on the organization view.
[e13] ux-check: hold the loading skeleton then let the settings resolve — the 'Enable spend alerts' row and the rule cards do not move
E2E proof — log excerpts
/home/igor_kilocode_ai/.local/share/kwf/sections/spend-alerts-optimize-ux-scopes-0d5a/e2e-mobile-app/scripted-e8.log/home/igor_kilocode_ai/.local/share/kwf/sections/spend-alerts-optimize-ux-scopes-0d5a/e2e-mobile-app/scripted-e9.log/home/igor_kilocode_ai/.local/share/kwf/sections/spend-alerts-optimize-ux-scopes-0d5a/e2e-web/web-e2e.logOwner request
[e9] mobile spend-alerts permission denied on the device — device account is a non-billing org member (db.sh: role 'member'); the org-scope screen shows 'Access denied' and 'You don't have permission to manage spend alerts.' with no settings controls (e9-permission.log); the org hub renders no 'Spend alerts' row for that role (e9-hub.log: 0 occurrences), so the described hub-row route does not exist for a member and the screen was reached by the app's own spend-alert deep link (e9-open.log: result ok, mode deeplink) — plan-route discrepancy, [pre-existing], consistent with the other billing rows hidden for the same member.
[e15] ux-check: mobile spend-alerts summary-group scope row and rule-card descriptions (personal and organization) — e15-personal.txt (+e15-personal.png): first summary-group row is 'Account'/'E2E Org Owner' with both rule descriptions; e15-org.txt (+e15-org.png): first row is 'Organization'/'Acme Corp' with the same two descriptions (e15.log); both visited screens captured. No UX-DEFECT observed.
[e15] ux-check: mobile spend-alerts summary-group scope row and rule-card descriptions (personal and organization)
[e9] mobile spend-alerts permission denied on the device
[e18] ux-check: configure and trigger a threshold crossing for an organization whose created_by_kilo_user_id is null and whose only owner holds the owner membership role — the owner receives the alert — android emulator-5554: signed in on the device as the org owner (e18-login.log) and switched to the org in Profile > account selector, then enabled spend alerts and saved a 24 h / $1 threshold from the org spend-alerts screen (e18-recipient.log: org:b0622be7-6122-4b60-bbcc-e7056e527516|true|threshold|true|1000000|24|true|false; screen e18-org-configured.png). The org row is 'b0622be7-6122-4b60-bbcc-e7056e527516|Acme Corp|NULL' and its only owner d839e489-… holds role 'owner'. The sweep tick fired 1 alert and the delivery row it enqueued named the owner (e18-recipient.log: recipients…
[e10] needs:fault: mobile spend-alerts save failure on the device — with the spendAlerts.save request failed, Save shows the save-error copy with a Retry and the edited draft is kept — android emulator-5554 (platform android): state settings STATE HIT (e10-state5.log); with nextjs up the form loaded ('SCENE e10 OK', e10-load.log: 'android.widget.TextView Account tappable [83,425][206,471]' and 'Email or push an alert to the billing contacts when this scope's own spend crosses a limit or spikes above its usual hourly rate. tappable [55,249][1025,341]', capture e10-loaded.png), then nextjs was killed ('fault.sh: nextjs killed (port 7100 refuses; pids 1691028 )', e10-fault-down.log) and Save logged 'SCENE e10 OK' with 'android.widget.TextView Couldn't save spend alerts.…
[e16] rendered spend-alert email names the scope and the kind — drain cron rendered fresh emails from db.sh-seeded delivery rows: personal shows 'Your account' + 'Spend threshold', org shows 'Acme Corp' + 'Spend threshold', org anomaly shows 'Acme Corp' + 'Hourly spike' (e16-email-.log); rows ended 'sent' (e16-deliveries.log); PNGs e16-email-.png captured for the visual reviewer; observation: an anomaly delivery with a null payload.thresholdMicrodollars fails 'spend_alert_delivery_missing_payload' (e16-anomaly-null-threshold.log), so the 'Hourly spike' email may not deliver when the sweep emits a null threshold.
Follow-ups (not changed here)