Skip to content

feat(activity): persist recipient attention items - #1517

Merged
lilyshen0722 merged 2 commits into
mainfrom
feat/task-112-attention-items
Sep 3, 2026
Merged

lilyshen0722 merged 2 commits into
mainfrom
feat/task-112-attention-items

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Materializes recipient-owned AttentionItem rows when a message mention, legacy Activity approval request, DecisionRequest, or concrete task handoff/blocked state is written.
  • Reads Needs you only from the indexed recipient rows and rechecks current pod membership before exposing snapshots. A task fact has no invented options: it opens its source thread.
  • Deletes the old read-time reconstruction (mentions, DecisionRequest, and board prose); activityService.ts shrinks from 1,669 to 1,339 lines. The retained recap suite keeps the six live authorization/acknowledgement protections active.
  • Retires the acknowledgement array and press-card UI; acknowledgement resolves the recipient-owned record.

Rollout

  • One-time direct-source backfill: 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

  • backend focused: 8 suites / 65 tests passed (source writers, task route boundaries, legacy approval authorization, model and decision coverage)
  • backend: npm run tsc:check
  • frontend: V2ActivityPage.test.tsx (11 passed) and tsc --noEmit
  • Rebased onto 05a9184e (current main) before request for review.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

SPEC GATE (TASK-112 row) — CHANGES NEEDED, three findings, all measured at pr/1517 vs origin/main:

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 getDecisionQueue — delete them, do not layer on top; activityService must shrink, not grow. At this head activityService.ts is 1,669 → 1,686 lines, and getMentionsForUser (:464), the DecisionRequest.find reader (:591) and the HANDOFF_RE board-row reader (:629) are all still there behind an early return. That is a layer on top with the old code left dead underneath. Delete them in this PR and put the before/after line count in the description.

2. Board rows lost their producer. The row lists task handoffs (HANDOFF_RE match → press) and blocked rows (→ decision) as facts that must be written to the table at the moment they happen. attentionItemService writes on mention, approval and DecisionRequest only, and the description says press cards are retired. Retiring the card is fine if the fact still lands in Needs-you; as written, a row going blocked or handing off to Sam no longer reaches him at all. Add the producer at the task write (status → blocked, or a HANDOFF_RE match on the last note) or say explicitly why that kind should die.

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 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 membership
  • fails closed when a non-member attempts a legacy Activity approval
  • fails closed when a non-member attempts a legacy Activity rejection
  • allows a pod member to approve a legacy Activity approval
  • does not mistake the approval.status default on an ordinary message for a request
  • removes 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

  1. Every message write now pays two Mongo round-trips whether or not anyone was mentioned. recordMentionedUsers calls currentHumanMembers(podId)Pod.findById + User.findbefore it looks at the content, then filters for @handle. The overwhelmingly common message has no mention and still pays both. A content.includes('@') guard before the lookup removes it. Worth raising because TASK-108 is already open on send latency (648–1030 ms measured).

  2. No test pins the invariant the model comment claims. AttentionItem.ts:49 says a retried source write "must not resurrect a recipient's acknowledgement". Mutating $setOnInsert$set does redden one test — but it is the mention-materialization test reacting to the operator's shape, not a behaviour test that a resolved row survives a re-write. The stated invariant deserves its own case: resolve, re-record, assert still resolved.

  3. getOpenQueue's count is capped at 80. count: valid.length is 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.

  4. A deleted schema field still has a reader. activityService.ts:470 selects activityQueue.acknowledgedMentionIds, which models/User.ts removes in this PR. It is unreachable in production — getDecisionQueue gates that block on ATTENTION_ITEM_LEGACY_READER_FOR_TESTS === '1' and otherwise delegates to getOpenQueue — 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 yields undefined, acked becomes an empty Set, and acknowledged mentions silently resurface. Deleting the block with the field would close it.

  5. docs/adr/ADR-017-attention-routing.md:377 goes stale with this merge. That paragraph is my own 2026-08-29 correction, asserting User.activityQueue.acknowledgedMentionIds is "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.

@samxu01
samxu01 force-pushed the feat/task-112-attention-items branch from 1e01a15 to 5387b8d Compare September 3, 2026 11:57

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at attentionItemService.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$set reddens does not resurrect a resolved attention row when its source write is retried directly, not by side effect on a shape assertion. That is the invariant AttentionItem.ts:49 claims, actually pinned.
  • Note 4 (deleted field, live reader) — zero readers of acknowledgedMentionIds remain under services/, routes/, models/. The backfill reads it through User.collection with an explicit comment that Mongoose would omit the retired path — exactly right.
  • Note 5 (stale ADR)ADR-017:379 now 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

  1. The new backfill has no inert-on-require test. It is inert — I ran a positive control with MONGO_URI unset and process.exit spied, and requiring it returns main without calling it. But __tests__/unit/models/migrationScriptIsInert.test.js hardcodes scripts/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 glob scripts/backfill-*.ts closes it for this script and the next one.

  2. The backfill's message reads are unbounded. Message.find({ createdAt: { $gte: cutoff } }) and the 14-day PG query both have no LIMIT, 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 --apply rather than changing the script.

  3. Carried from the last gate, still open: getOpenQueue's count: valid.length is 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.

@lilyshen0722
lilyshen0722 merged commit 77b8857 into main Sep 3, 2026
17 of 21 checks passed
@lilyshen0722
lilyshen0722 deleted the feat/task-112-attention-items branch September 3, 2026 17:13
lilyshen0722 added a commit that referenced this pull request Sep 3, 2026
#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant