Fix #1472: bound the mailbox escalation-toast dedupe set - #1484
Open
mohidmakhdoomi wants to merge 10 commits into
Open
Fix #1472: bound the mailbox escalation-toast dedupe set#1484mohidmakhdoomi wants to merge 10 commits into
mohidmakhdoomi wants to merge 10 commits into
Conversation
The `seen` Set in `activateMailboxEscalationToasts` was add-only and activation-scoped: one permanent entry per escalated mailbox row for the lifetime of the extension host. Evict on the row leaving the escalated set — the same signal that would let a legitimate re-escalation re-notify, mirroring how `activateGateToasts` prunes a builder that leaves the blocked set. The overview exposes no per-row mailbox ids, only the workspace-level `mailboxEscalated` flag, so `false` (nothing in this workspace is escalated) is the finest-grained signal available client-side: at that point every id in the set has left it, and the set is dropped whole. A MAX_SEEN=500 oldest-first cap backstops a window that stays escalated all day and never observes that `false`. Regression tests pin both paths (verified failing without the fix): an id re-toasts after de-escalation, and the cap evicts oldest-first while the workspace never de-escalates. Also covered: no prune while still escalated, and no prune on an empty cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CMAP non-blocking finding (claude lane): Tower also reports `mailboxEscalated: false` when it cannot read the mailbox, so the doc comment's "no held row is escalated" was marginally stronger than the server guarantees. Say so, and why it is harmless — a row escalates exactly once server-side and there is no SSE replay, so an early prune leaves no second event to dedupe against. Also commits the builder thread with the CMAP verdicts and the disposition of each non-blocking note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Architect integration review (risk tier: Low — isolated VSCode module)Verdict: APPROVE (pending maintainer review — we are not maintainers; maintainer merges.) Verified directly against the diff:
Builder CMAP: gemini/codex/claude all APPROVE. No findings to address. Parked for maintainer approval + merge. |
The pr gate was approved and porch reports protocol complete. Porch's final merge task is deliberately skipped: a maintainer merges this PR, not the builder. GitHub concurs — REVIEW_REQUIRED / BLOCKED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The VSCode mailbox escalation toast deduped "already shown" escalations with a
seenSet that only ever grew — one permanent entry per escalated mailbox row for the lifetime of the extension host. It is now bounded, evicting on the mailbox row leaving the escalated set.Fixes #1472
Root Cause
apps/vscode/src/notifications/mailbox-escalation-toast.ts—const seen = new Set<string>()is activation-scoped, andseen.add(payload.mailboxId)was its only mutation: nodelete, noclear, anywhere in the module. Add-only container + extension-host lifetime = unbounded growth. Negligible per entry, but it never drains.Fix
Evict on the mailbox row leaving the escalated set — the same signal that would let a legitimate re-escalation re-notify — mirroring how
activateGateToastsprunes a builder that leaves the blocked set:OverviewCache(already in scope at the single call site inextension.ts) and subscribes toonDidChange.OverviewDatacarries no per-row mailbox ids, only the workspace-levelmailboxEscalatedflag, sofalse— nothing held in this workspace is escalated — is the finest-grained "left the escalated set" signal available client-side. At that point every id in the set has left it, so the set is dropped whole. A null/absent cache read says nothing about the escalated set and never prunes.MAX_SEEN = 500with oldest-first eviction (aSetiterates in insertion order) backstops the pathological window that stays escalated all day and so never observes thatfalse.No protocol or server change; the module and its call site are the whole surface.
Two notes checked against the server rather than assumed:
db/mailbox.tssetsescalated = 1once, guarded byescalated = 0, and never resets it; terminal rows are pruned. So the samemailboxIdcannot legitimately re-escalate — eviction here is purely about memory, never about suppressing a notification.OverviewCache.refresh()is last-write-wins by sequence, and the escalation SSE event itself triggers a refresh whose request starts after Tower flagged the row, so an older in-flightmailboxEscalated: falseresponse can never commit after it.Test Plan
src/__tests__/mailbox-escalation-toast.test.ts: an id re-toasts after de-escalation; the cap evicts oldest-first while the workspace never de-escalates; no prune while still escalated; no prune on an empty cache. Verified failing without the fix (2 failed / 10 passed with the eviction neutered) and passing with it (12/12).pnpm check-typesclean; porchbuildcheck green.pnpm lintreports one pre-existing warning in the unrelatedsrc/commands/tunnel.ts.🤖 Generated with Claude Code