Resolved threads stop nagging

[HamptonMakes]

The bug

Open the workspace in production today and the "Needs attention" strip claims eleven unread comments on one plan and twenty on another. Click through and the plan looks empty — because those comments were all resolved. An agent replied to a batch of threads and then resolved them, and each reply plus each resolve minted an inbox row that outlived the thread it pointed at. The doc view hides a closed thread's highlight (.anchor-highlight--resolved is pointer-events: none until you press s), so the rows sent you somewhere with nothing to act on.

The rule

A closed thread — resolved or discarded — carries no unread notifications.

• Closing sweeps. CommentThread gained an after_commit that calls Notifications::MarkThreadRead when a status write lands in a closed status. It lives on the model rather than in the two controllers that resolve/discard (web + API), so seeds, admin, and any future path are covered too. Rows stay in the inbox history; they just stop counting as unread.
• Nothing new gets minted. Notifications::Create::SILENT_ON_CLOSED_THREAD drops agent_response and status_change on an already-closed thread. Human reply and mention still notify — somebody deliberately re-engaging with a settled thread is news, and that's the one signal I didn't want to swallow. Flagging this as the judgement call in the PR: the trade is that when an agent resolves a thread, you no longer get told. Say the word and I'll make status_change-on-resolve survive.
• The existing pile gets cleared. MarkClosedThreadNotificationsRead marks read every unread row whose thread is already closed. Data-only, idempotent, so db/schema.rb moves by one version line.
• Reopening still notifies. reopen sets pending before the job runs, so the thread is open and the status_change lands.

Getting rid of the rest

Opening a plan clears that plan's unread rows. You looked; the nudge is done. Notifications::MarkPlanRead.

The signal for "opened" is PlanPresenceChannel#subscribed, not the GET. Workspace rows opt into Turbo 8 hover prefetch, which makes the request unreliable in both directions: resting the cursor on a row fetches the page (so clearing there would quietly empty the strip for a visit nobody made — I hit this in the browser), and a click within 10s is served from Turbo's prefetch cache without touching the server at all. The presence subscribe only happens when the page is live in front of somebody. PlansController#show clears too, for navigations Turbo didn't prefetch, and skips X-Sec-Purpose: prefetch — which also stops a hover from advancing your last-seen mark and burning the changed-section highlights, a pre-existing bug of the same shape.

And a ✕ per row, for plans you don't want to open. It:

• marks that plan's unread rows read
• re-renders the strip in place, because clearing one plan can promote the next one into the top five
• removes that plan row's own unread badge (a row still reading "3" under a cleared strip reads as a stale page)
• refreshes the bell

No navigation, no scroll jump, no full reload; the row hides the moment you press it rather than narrating "Clearing…". Verified in a browser against a seeded worktree DB: hover a row → nothing clears; click it → its rows clear and the bell drops live over the existing notifications stream; ✕ on the rest → strip empties and disappears rather than leaving an empty bordered box.

Refactors this pulled in

• The strip moved out of plans/index.html.erb into plans/_needs_attention.html.erb behind Notifications::NeedsAttention, so the workspace and the Clear response render the same thing from the same query object. It also now drops a row whose notification vanished mid-request instead of raising on fetch.
• The badge-broadcast block that was copy-pasted in Notifications::Create, Comments::ProcessMentions, and NotificationsController is now Notifications::BroadcastBadges.

Testing

bundle exec rspec — 1538 examples, 0 failures. New coverage: the close sweep (model, web resolve/discard, API resolve), the reply-then-resolve race (fails without the thread lock), the silencing rule including the reply/mention/reopen exceptions, NeedsAttention (ordering, cap, visibility predicate, mid-request clear, not built for pagination frames), clear-on-open (plan show clears the viewer's rows and nobody else's, ignores a prefetch, doesn't advance last-seen on a prefetch, presence subscribe clears, rejected subscription doesn't), mark_plan_read (scoping to the acting user, turbo-stream targets, no-op without a plan_id), and the migration (idempotent, leaves open threads and already-read rows alone). Also confirmed the reflected plan_id is escaped in the turbo-stream target attribute.

Still open

The bigger question you raised — whether a local agent writing one comment should push at all — isn't touched here. Web Push still fans out per notification created; the only change is that fewer notifications exist to push. Worth its own pass on per-reason delivery rules.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b4e2d5313

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[HamptonMakes]

🤖 Both Codex findings addressed in e40929b.

P1 — no feedback while clearing. Fair catch against the native-feel bar in AGENTS.md. The Clear button now carries data-turbo-submits-with="Clearing…", so its label swaps and it disables the instant it's pressed, and the row pulses while Turbo has the form marked aria-busy (.attention__item:has(.attention__clear-form[aria-busy="true"]), following the existing dnd-drop--busy convention). Turbo restores both the label and the attribute on failure, so no JS of our own and no stuck state. Verified in a browser: the pulse animation and cursor: progress activate exactly on aria-busy, and the click still lands cleanly.

P2 — extra queries on pagination frames. Correct, and a regression I introduced: unread_by_plan routed through NeedsAttention, so a Turbo-frame page fetch — which returns before the strip renders — paid for the per-plan notification lookups and the visible-plan query too. unread_by_plan is its own memoized grouped count again, and NeedsAttention accepts that hash as an argument so the workspace still runs it exactly once. Added a request spec asserting NeedsAttention is never built for a pagination frame, so this can't quietly come back.

Full suite green: 1521 examples, 0 failures.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e40929b2ec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[HamptonMakes]

🤖 Correction on the P1 fix in 11e153c — I took the wrong half of that suggestion.

"Clearing…" was a progress state for an action that has no duration. Dismissing a nudge should just dismiss it, so the row now hides the instant Turbo marks the form aria-busy, and the Turbo Stream response replaces the strip behind it. No label swap, no pulse. Clearing the last row takes the whole strip with it rather than flashing an empty bordered box with a stale count.

Failure still self-heals: Turbo clears aria-busy when a submission fails, which puts the row back — no JS of ours either way.

Verified in a browser: with two rows, the clicked row goes display: none while the other stays; with one row left, the row and the strip both go. Suite still green at 1521 examples.

[HamptonMakes]

🤖 Good catch on the race — fixed in bb4c7ed, and it was worse than a stuck row.

WebPushDeliveryJob doesn't re-check read state, so a notification inserted after the sweep both stays unread on a closed thread and delivers a push about a thread that's already resolved. That's the exact complaint this PR exists to fix, so preventing the insert (rather than sweeping again afterwards) was the right shape.

Reading the status and inserting are now one step inside @comment_thread.with_lock, which reloads the thread. Either order is safe:

• resolve commits first → the lock's reload sees resolved → silent, no row, no push
• the job gets the lock first → the resolve's UPDATE waits, commits after us, and its own after_commit sweep catches the rows we just wrote

The badge broadcast stays outside the lock, and the notification's after_commit push fan-out now fires on the outer transaction, so only rows that actually survive can push.

The spec reproduces it deterministically — a stale in-memory thread plus a committed resolve — and fails without the lock with two stuck rows. Suite: 1522 examples, 0 failures.

[HamptonMakes]

🤖 > I was mostly hoping that opening the plan would clear them.... though an "x" is good too.

Flipped it round — opening the plan is now the main way rows clear, and the dismiss is a quiet ✕ (matching the filter chips) rather than the word "Clear". 6d4d02b.

The wrinkle worth knowing about: "opened the plan" can't be the GET /plans/:id. Workspace rows opt into Turbo 8 hover prefetch (data-turbo-prefetch: true in _plan_row.html.erb), so:

1. Resting the cursor on a row fires a real request 100ms later. My first cut cleared notifications there — I caught it in the browser when a plan I'd never opened lost its four unread rows because my mouse happened to be parked on it. (Same shape of bug already existed for PlanViewer.track: a hover advanced your last-seen mark and burned the "changed since you last looked" highlights.)
2. Clicking within 10s is served from Turbo's prefetch cache — the server never sees the navigation, so a guard alone would mean the click never cleared anything.

So the "you actually looked at it" signal is PlanPresenceChannel#subscribed, which only fires when the page is live in a browser. PlansController#show still clears for navigations Turbo didn't prefetch (deep links, reloads, cold clicks), and skips X-Sec-Purpose: prefetch — which also stops a hover from burning the changed-section highlights. Notifications::MarkPlanRead is shared by both paths and the ✕.

Verified in a browser on a seeded DB: hover a row for 2s → request fires, 9 unread stays 9; click it → plan opens, its 3 rows clear, bell goes 9 → 6 live over the existing notifications stream with no reload; ✕ on the remaining rows clears them in place and the strip disappears when the last one goes.

bundle exec rspec — 1538 examples, 0 failures. New coverage: plan show clears the viewer's rows (and nobody else's, and not on the history page), the prefetch header clears nothing and doesn't advance last-seen, the presence subscribe clears (and doesn't when the subscription is rejected), and MarkPlanRead scoping.