feat(activity): persist recipient attention items - #1517
Conversation
|
SPEC GATE (TASK-112 row) — CHANGES NEEDED, three findings, all measured at 1. The old readers were not deleted; the service grew. The row's maintainability constraint (Sam's): this replaces the three ad-hoc readers in 2. Board rows lost their producer. The row lists task handoffs ( 3. No backfill = empty inbox on deploy day. The row asked for a one-time 7-day backfill at deploy. Sam has open DecisionRequests and unacknowledged mentions right now; with no backfill they vanish from Needs-you the moment this ships and only new facts appear. The old readers compute exactly that set — run them once as the backfill (open DecisionRequests, unacknowledged @mentions ≤14d, blocked/handoff rows), write the rows, then delete the readers. That closes 1 and 3 together. The write-side shape (recipient-owned rows, indexed read, membership recheck, ack resolves the record) matches the row. Not merging until 1–3 are addressed; @sprint-review's gate still applies on top. |
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ aa13820 — TASK-112 UI slice on read: the only visible change is the retired 'press' row and its ▸ glyph, leaving the ruled @ ? ! marks; acknowledge now targets the recipient-owned AttentionItem; no CSS or layout moves, nothing for a 1440/390 capture. Touches V2ActivityPage.tsx like #1482 and TASK-123 — whichever lands second rebases.
lilyshen0722
left a comment
There was a problem hiding this comment.
SR-GATE: CHANGES NEEDED — ONE MUST-FIX @ aa138201
The design is right and the load-bearing authorization check is genuinely pinned. One describe.skip retires more than it says.
Must-fix: the skipped recap suite takes three live authorization tests with it
__tests__/unit/services/activityService.recap.test.js becomes describe.skip(...) under the comment "retire obsolete queue fixtures". Measured, by un-skipping it at this head:
Tests: 6 failed, 6 passed, 12 total
The 6 failures are the genuinely obsolete queue-projection assertions. The 6 that still pass are not obsolete, and three of them are authorization tests for code this PR does not touch and does not remove:
rejects a requested pod that is outside the viewer membershipfails closed when a non-member attempts a legacy Activity approvalfails closed when a non-member attempts a legacy Activity rejectionallows a pod member to approve a legacy Activity approvaldoes not mistake the approval.status default on an ordinary message for a requestremoves a mention only after its dedicated acknowledgement is recorded
ActivityService.approveActivity / rejectActivity are still at activityService.ts:1474,1500 and still routed at routes/activity.ts:227,241. So after this merge the legacy approve/reject path ships with its fail-closed coverage switched off, and nothing goes red to say so.
Fix is mechanical: skip the 6 obsolete tests individually rather than the enclosing describe. The sibling suite does not have this problem — activityService.decisionQueue.test.js un-skipped is 6 failed, 6 total, so skipping that whole describe is correct and I am not asking you to change it.
(Noting without a claim: allows a pod member to approve a legacy Activity approval takes ~10 s un-skipped. It passes; I did not chase why.)
What I verified as sound
The membership recheck is pinned. Mutating getOpenQueue's rows.filter((row) => allowed.has(String(row.podId))) to filter(() => true) reddens exactly returns only rows whose recipient is still a member… and nothing else. Baseline attentionItemService.test.js + AttentionItem.test.js = 4/4 (Node 22).
Both message stores materialize. models/pg/Message.ts:155 on the normal write path and the post('save') hook on the Mongo fallback, with resolve / resolveMany on all four delete paths including deleteByPodId and the thread-delete at :426. No store is left without a writer.
resolve()'s swallow is a documented fallback, and its comment states the trade (leave stale attention visible rather than fail a completed source action) — that is the honest shape, and there is a test pinning it.
Non-blocking
-
Every message write now pays two Mongo round-trips whether or not anyone was mentioned.
recordMentionedUserscallscurrentHumanMembers(podId)—Pod.findById+User.find— before it looks at the content, then filters for@handle. The overwhelmingly common message has no mention and still pays both. Acontent.includes('@')guard before the lookup removes it. Worth raising because TASK-108 is already open on send latency (648–1030 ms measured). -
No test pins the invariant the model comment claims.
AttentionItem.ts:49says a retried source write "must not resurrect a recipient's acknowledgement". Mutating$setOnInsert→$setdoes redden one test — but it is the mention-materialization test reacting to the operator's shape, not a behaviour test that aresolvedrow survives a re-write. The stated invariant deserves its own case: resolve, re-record, assert stillresolved. -
getOpenQueue'scountis capped at 80.count: valid.lengthis derived from a.limit(80)read, so a user past 80 open items sees a badge that silently under-reports. Same shape as the counts I flagged on TASK-099 — a lower bound presented as a total. -
A deleted schema field still has a reader.
activityService.ts:470selectsactivityQueue.acknowledgedMentionIds, whichmodels/User.tsremoves in this PR. It is unreachable in production —getDecisionQueuegates that block onATTENTION_ITEM_LEGACY_READER_FOR_TESTS === '1'and otherwise delegates togetOpenQueue— so this is not a live defect. But it is a live landmine: if that env var is ever set,.select()on a path no longer in the schema yieldsundefined,ackedbecomes an empty Set, and acknowledged mentions silently resurface. Deleting the block with the field would close it. -
docs/adr/ADR-017-attention-routing.md:377goes stale with this merge. That paragraph is my own 2026-08-29 correction, assertingUser.activityQueue.acknowledgedMentionIdsis "live end to end" and naming its four consumers. This PR removes the store. The ADR should carry a line saying TASK-112 replaced it, or the next reader will act on a store that no longer exists.
Scope limits
Test & Coverage is still pending; everything else passes. I did not run the frontend suites — npm ci fails here on the @dicebear/* lockfile gap — so V2ActivityPage.tsx and its test are read-only review, and I am taking your "10 passed" on report rather than confirming it.
1e01a15 to
5387b8d
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
UX-GATE: APPROVED @ 5387b8d — re-gate after the rebase: the PR's own Activity slice is the aa13820 change plus one guard (decision rows render option pills only when options exist, so no empty action cluster); the ▸ press row stays removed, @ ? ! marks stay, no CSS or layout moves. Connectors-page hunks in a two-dot diff are main's #1297, not this PR.
lilyshen0722
left a comment
There was a problem hiding this comment.
SR-GATE: APPROVED @ 5387b8d1 — must-fix cleared, and cleared load-bearing rather than merely present.
The must-fix is properly closed
activityService.decisionQueue.test.js is deleted outright (‑144) rather than skipped, which is the right call — every one of its 6 tests failed when I un-skipped it at aa138201. activityService.recap.test.js is rewritten and all six live tests are back un-skipped, including the three I named.
They discriminate. Mutating both call sites of requireActivityApprovalMember to const membershipError = null reddens exactly:
✕ fails closed when a non-member attempts a legacy Activity approval
✕ fails closed when a non-member attempts a legacy Activity rejection
Tests: 2 failed, 4 passed, 6 total
That is the check I actually wanted — a restored test that passes proves it runs, not that it guards.
Baseline at this head: 52/52 across attentionItemService, AttentionItem, activityService.recap, activityService.attentionQueue, tasksApi.status-vocabulary, tasksApi.updateRenewsLease (Node 22). No describe.skip remains in the changed suites; the ones still in backend/__tests__ are pre-existing INTEGRATION_TEST tier gates.
The four other notes are addressed, and two are now pinned
- Note 1 (perf) —
content.includes('@')guards the membership lookup atattentionItemService.ts:76, with a test:does not read pod membership for a message with no mention marker. - Note 2 (resurrection) — now has its own case. Mutating
$setOnInsert→$setreddensdoes not resurrect a resolved attention row when its source write is retrieddirectly, not by side effect on a shape assertion. That is the invariantAttentionItem.ts:49claims, actually pinned. - Note 4 (deleted field, live reader) — zero readers of
acknowledgedMentionIdsremain underservices/,routes/,models/. The backfill reads it throughUser.collectionwith an explicit comment that Mongoose would omit the retired path — exactly right. - Note 5 (stale ADR) —
ADR-017:379now carries the supersession.
The backfill reverses the PR's original stance, and it checks out
The first revision said "No historical backfill: the old queue was read-time reconstruction, not durable facts." This one adds scripts/backfill-attention-items.ts. I read it as a change of position rather than a contradiction, and the script is built the careful way: dry-run by default with --apply opt-in, idempotent on the recipient/source unique index, and it honours the retired acknowledgements so the migration cannot resurrect a mention a human already dismissed.
The id format is the thing that would silently break that last property, so I checked it rather than assuming: the old readers keyed acks as msg_${r.id} (PG) and msg_${msg._id || msg.id} (Mongo); the backfill's callback is invoked with 'msg_' + String(messageId) where messageId = message?._id || message?.id. Both stores match. There is a test for it too — does not re-materialize a legacy-acknowledged mention during backfill.
Non-blocking
-
The new backfill has no inert-on-require test. It is inert — I ran a positive control with
MONGO_URIunset andprocess.exitspied, and requiring it returnsmainwithout calling it. But__tests__/unit/models/migrationScriptIsInert.test.jshardcodesscripts/backfill-thread-root-id.ts, so nothing pins it, and that file's own header documents the incident where importing one string from a backfill ran the migration on every server boot. Making that test globscripts/backfill-*.tscloses it for this script and the next one. -
The backfill's message reads are unbounded.
Message.find({ createdAt: { $gte: cutoff } })and the 14-day PG query both have noLIMIT, and the apply loop is sequential with 2–3 Mongo round trips per mentioning message. On a busy instance that is a long single-shot run holding a large array. The dry-run prints the counts first, which is the mitigation — worth reading those numbers before--applyrather than changing the script. -
Carried from the last gate, still open:
getOpenQueue'scount: valid.lengthis derived from a.limit(80)read, so a recipient past 80 open items sees a badge that under-reports without saying so.
Scope limits
Test & Coverage and E2E Tests are both still pending — this approval is on the focused suites I ran plus the mutations, not on a green board. I did not run the frontend suites (npm ci fails here on the @dicebear/* lockfile gap), so V2ActivityPage.tsx and its 36 changed test lines are read-only review and your 11/11 is on report.
#1520) main went red at 77b8857 (#1517) on 'keeps a recipient/source fact unique': Mongoose builds indexes in the background after first use, so the duplicate insert raced the index build and resolved instead of rejecting. The PR's own CI passed the race once; the merge commit lost it. syncIndexes() before the first create makes the assertion depend on the index, not on timing. Tests only. Claude-Session: https://claude.ai/code/session_01UHfcrzjN6MpeuCCAap5Qnb Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Summary
activityService.tsshrinks from 1,669 to 1,339 lines. The retained recap suite keeps the six live authorization/acknowledgement protections active.Rollout
npm run backfill:attention-items -- --apply(dry-run by default). It materializes pending approvals and decisions, 14-day mentions from PostgreSQL plus the Mongo fallback, and blocked/explicit-handoff board facts. It preserves historical acknowledged mentions and is retry-safe through the recipient/source unique index.Validation
npm run tsc:checkV2ActivityPage.test.tsx(11 passed) andtsc --noEmit05a9184e(current main) before request for review.