fix(activity): make the recap actionable - #1298
lilyshen0722 wants to merge 3 commits into
Conversation
|
UX gate (ux-lead) at Blocking: the 1280/390 captures are fixture data (release-agent / security-agent, TASK-412, "Launch readiness"). TASK-083 was measured by Sam on prod, and the gate asked for captures on Sam's real account so we can see the actual outcome: default scope populated, no commonly-bot card, ≥1 press/decision row derived from the live board. A fixture proves the render, not the fix. Please re-capture against prod data (dev deploy or local pointed at prod API) and attach. Verified in the diff and fine to ship as-is: Non-blocking nits, follow-up or now:
|
lilyshen0722
left a comment
There was a problem hiding this comment.
sprint-review gate — BLOCK. Head e750a17e92e71b1d907d0256285fd5e0f2e42512, 10 files (+594/-63), mergeable_state: dirty, behind = 52.
The shape of this is right — the recap should say what needs a human, and deriving the active-pod set from a grouped PG read instead of the feed's first 100 rows is the correct fix for the right reason. findSubstantiveAgentPodActivity catches and returns [], so the feed fallback still works when PG is down. The 'all' scope does not escape membership: scopedPods = memberPods, and memberPods is already the createdBy/members.userId query.
The classifier has no polarity, and it fails toward "press it".
taskAttentionKind returns 'press' when a task's combined title/notes/updates text matches PULL_REQUEST_REFERENCE and GATED_PULL_REQUEST. Neither regex encodes a verdict — they match the vocabulary of gating. I ran them over text I have actually posted in the pod this PR serves:
press <- "#1355 re-gated at 630286c5 — BLOCK stands. The admin lockout is untouched."
press <- "#1218 gated: substance right, BLOCK — the version it bumps to is already published."
press <- "PR #999 is not gated; changes requested."
press <- "#1234 is NOT approved — do not press"
press <- "#1354 gated PASS, CLEAN, ready to merge" <- the only true positive
\bgated\b matches inside re-gated, because the hyphen is a word boundary. It does not match inside ungated, because that one is a prefix. That distinction is accidental rather than designed, which is the tell that the boundary is not carrying any meaning here.
The consequence runs opposite to the feature's purpose. A blocking verdict contains strictly more gating vocabulary than a passing one — it names the PR, quotes the gate, and explains the refusal — so a queue built to surface "what needs you" will preferentially surface blocked PRs as ready to press. The failure is silent at the point it matters: the human sees a press affordance, not the sentence that produced it.
The suite does not see this because every fixture is a true positive. 'Gated #1274 — awaiting human press.' is correctly classified, and nothing asserts a negative — no BLOCK verdict, no refusal, no negation. A classifier needs a real member of each population it must separate; this one has only one population.
I do not think a better regex is the fix. Polarity is not recoverable from prose that discusses gating, and near-misses here are costly in one direction only. The signal already exists as structure: a review's state, the PR's mergeable_state, the task's own status. If the text path stays, it should require an affirmative marker the writer opts into, and press should be the classification that needs the strongest evidence rather than the one reached by default.
The two new layout invariants do not guard what their comment says they guard.
v2.css contains two @media (max-width: 640px) blocks, at lines 7434 and 7951. The new assertions anchor on the literal and then walk with [\s\S]*?, so the match starts at 7434 and runs ~530 lines — across the end of the first block and through unmediated CSS — to reach the rule at 7966. Mutation-checked both directions, with the control:
- delete the rule outright → both assertions red (so they are not inert; they do pin its existence)
- move the rule outside every media query, unchanged → both assertions still green
So a future edit that hoists grid-template-columns: minmax(0, 1fr) minmax(0, 120px) out of the media block ships green, and it is the mobile scoping — "still needs to be usable at 390px" — that the comment claims to be defending. The pre-existing .v2-activity__board-row assertion at :447 has the same shape, so this is a copied pattern rather than a new mistake, but these are two new claims and they should hold. Extract the second media block's body and match inside it.
This matters more than usual here because the presence test is standing in for a browser check: jsdom has no layout engine, so nothing else in CI can see a 390px regression.
Merge blockers. Four conflicts — backend/services/activityService.ts, both i18n/locales/*.json, and frontend/src/v2/components/V2ActivityPage.tsx — plus behind = 52 against MAX_BEHIND: 40. The green stale-base tick predates 52 commits of main and is expired; it fires only on [opened, synchronize, reopened, edited].
One non-blocking asymmetry. In the activity filter, requestsAllPods returns true for every row unconditionally, while the default path filters on !activity.pod || scopedPodIds.has(...). That is safe today only because getUserFeed(userId) is already user-scoped — the 'all' branch is doing no scoping of its own. Worth a comment saying so, since the next person widening getUserFeed would have no reason to look here.
Fixes TASK-083.
activeInWindowandagentMessageCount; the native selector groups active and other pods, with explicit All pods.Proof:
backend: npm test -- --runInBand __tests__/unit/models/PgMessage.test.js __tests__/unit/services/activityService.recap.test.js— 19 passedV2ActivityPage+ layout-invariant suites — 61 passed; frontend typecheck and production build passedKnown baseline: repo-wide lint remains pre-existingly red (missing CLI eslint plus unrelated parser/import/i18n errors); changed files' focused behavior and build checks are green.