fix(gmail): the thread you are looking at becomes the thread you are talking about - #54
Conversation
sebastian-ssvlabs
left a comment
There was a problem hiding this comment.
Reviewed the PR's own 4 commits (3ccbab9..8c973a4, 12 files, +1105/−188): the bridge/chip/content-port split, the observer, ComposerPills, ChatColumn, and the browser.rs trace.
Two medium findings inline — both cases where a key or a flag outlives the thing it described. Seven lower-severity ones I've left off the diff to keep this readable; happy to post them if useful. Two are worth naming because they are tests that no longer test what they claim:
report.test.ts:209—matchAll(/\["(\w[\w-]*)",\s*"/g)needs[immediately followed by", which the reformatted multi-lineITEMSentries no longer satisfy. The extraction now yields exactly["tag"], so "the menu offers only verbs the host will accept" covers only the row that is hidden and no longer checksdraft-updatesorcatch-up— the two actually drawn.report.test.ts:172—source.slice(source.indexOf("function onContextMenu"))returns the last character rather than""when the function is absent, soexpect(handler, "onContextMenu not found").not.toHaveLength(0)can never fail.
The rest: ComposerPills.svelte:212 computes groundingShownInScope without regard to wsRoom while the ✕ only renders in the wsRoom branch (latent — only Gmail sets onDismiss today, always in a workstation room); ComposerPills.svelte:341 adds three CSS rules (.scope-type, .scope-label, .scope-dismissable) that match nothing, since the markup uses .ty/.tx; browser.rs:167 uses env::var(...).is_ok(), so BRAINS_BROWSER_TRACE=0 turns the trace on; and gmail-observer.js:501 resyncs on every pointerdown, which on the OS-webview transport makes each click in the mailbox a window.open sentinel the host must refuse plus a duplicate report emit.
Clean on inspection: report-handler.ts, content-port.ts, openThread()/onScreen(), and the capture→bubble blur fix — that diagnosis and the visible-heading-only rule both hold up, and the new unit tests cover the interacting dismissal rules well.
… that survives its own click
Three defects in the injected observer, all of which looked the same from
outside: the thread you clicked never reached the chat.
DELIVERY. report() recorded a snapshot as sent before sending it, and send() is
fire-and-forget by construction -- the only transport is a refused popup at the
sentinel, so a report emitted while the host was still subscribing is
indistinguishable from a delivered one. Setting `last` anyway made that loss
PERMANENT for that exact state: the same thread stayed dead forever while a
different one worked. resync() drops the memo on a real user action, which is
bounded by how fast a person can click and makes the gap self-healing. Not
inside our own menu, though: that pointerdown is one step from a click that
sends a pointed report, and Chromium spends transient user activation on the
first window.open.
THE VISIBLE CONVERSATION. Gmail caches conversation views in the DOM, hidden, so
querySelector("h2[data-legacy-thread-id]") returned a thread the user had closed
-- in another mailbox. Measured live: 2 headings present, 1 visible, in a Sent
list with nothing open. Worse, the stale node does not change as the user clicks
around, so the snapshot never changed and the dedupe swallowed every report.
openThread() now takes only an on-screen heading, preferring the one the hash
names, and reads the id off that heading -- which also fixes the thread prefetch,
since threadIdFromHash accepts hex only and a modern Gmail hash carries FMfcgz
permalink ids it rejects.
THE MENU. addEventListener("blur", removeMenu, true) -- blur does not bubble, so
a capturing listener on window is not "the window lost focus", it is every
element blur in the page. Clicking a menu row moved focus off the Gmail row
behind it, that row blurred, and the menu was torn out between mousedown and
mouseup, so no click ever dispatched. That is why the first right-click did
nothing and the second worked: focus had already left. Non-capturing now, plus
rows that preventDefault their own mousedown so focus never moves at all.
Guarded by a test that fails when the capture flag goes back. The existing
preventDefault-ordering test is scoped to onContextMenu, because the menu rows
now legitimately cancel their own mousedown and a file-wide indexOf found that
one first and read the order backwards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GmailTab went 393 -> 546 lines carrying this branch, over the 400 soft ceiling (CLAUDE.md rule 3). Rather than shuffle lines to satisfy a warning, the parts that have actually been WRONG move out, and they are pure: bridge/report-handler.ts what a report means chip.ts what the composer's chip says it is about content-port.ts what a skill can read Every rule in the first two shipped broken at least once this week -- which thread wins when the grounding and the screen disagree, whether a dismissed one may come back, whether a passive report may promote itself, what a mailbox showing a list should do. None of it was reachable by a test while it lived in an $effect wired to a live WebView. It is now 28 assertions with no DOM at all, and two mutation checks confirmed they bite. content-port.ts is bulk rather than bugs, but it is a coherent unit with its own contract and it is what brings the file under the ceiling. A factory over getters instead of a closure: read() is called on demand, once, when a skill asks -- so the values only need to be current at call time, which a getter gives without dragging reactivity into a module with no other use for it. GmailTab 379 lines, and it now subscribes, applies and tears down; it decides nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The composer drew the thread twice under two different labels: the scope breadcrumb said "Inbox > subject" and the promoted canvas selection drew a second pill saying "Thread: subject". When they disagreed -- right-clicking one row while another is open, which is the ordinary case -- the user saw two chips naming two different threads. One chip now, reading "Gmail: <subject>" and carrying its own X. "Gmail" rather than "Inbox" is correctness, not preference: the user moves between Inbox, Sent, Drafts and searches, so a hardcoded "Inbox:" was wrong in all but the first, and the report carries no mailbox name to do better with. The app is the thing that does not change as they navigate. The grounding still exists and still grounds the turn -- ComposerPills only stops DRAWING its own pill when a dismissable scope chip is present. Keyed on the dismiss control rather than on the two titles matching, because matching titles was the first attempt and it failed in exactly the case that matters: the titles differ by definition there. Cost: a grounding carrying an excerpt no longer shows that quoted text anywhere. Nothing reaches that path via right-click today; worth revisiting if selections inside a thread become a thing people do. Long subjects: the chip clips head-weighted with the full text in a title attribute, and the composer placeholder is clipped in JS because a placeholder is a string, not a text node, so CSS cannot ellipsize it -- a mail subject wrapped it to three lines before a character was typed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
on_new_window is the only place that can say whether a report the page believes it sent ever arrived. The page cannot: send() is a refused popup, so a delivered report and one that reached nobody are identical from in there. The webview's own devtools are not reachable in the dev app either. Three wrong guesses went into the right-click defect before this existed -- the availability gate, listener ordering, then user activation, each costing a rebuild. With the trace on, the answer came back in one round and was none of them: two passive reports, zero pointed ones, which said the click handler was never running at all. Off unless BRAINS_BROWSER_TRACE is set. Prints the action when there is one, because a passive report arriving while a pointed one goes missing is a very different fault from nothing arriving. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…once Three from review, all cases where a key or a flag outlived the thing it described. **The dismissal was keyed off the wrong thread.** Arriving reports are keyed by `dismissalKey` off the thread the observer says is OPEN; `threadKeyFor` keyed off the GROUNDED one. They agree until they don't — right-click row B while thread A is open — and then the ✕ either never matched (no id, which is the common case: the hold carried B's subject, every report carried A's, and the chip returned naming A a quarter-second later) or muted the wrong conversation (id present: the hold took A while the chip read B, so it looked like it worked). The key now follows the open thread, which is what the reports carry. The label still comes from the effective thread — only the key moved. **The gate opened on the cancelled path.** A tab switch tears the effect down and starts another, but the old promise still resolves, and `listening = true` ran there too — so the panel mounted while the live subscription was still pending. That is precisely the lost first report this gate was added to prevent, reintroduced by the gate itself. Only the subscription that survived may open it, and a subscribe that fails after teardown no longer opens it at all. **And the mock never released a dismissal.** `dismissedThreadId` was only cleared from the report listener, which a mock tab never receives, so a thread dismissed there stayed suppressed for the whole session and reopening it never brought the chip back. Mock navigation now releases it, which is the same rule live mode gets from the report stream.
`.scope-type`, `.scope-label` and `.scope-dismissable` are no longer referenced by anything in this file — the chip's markup moved when its rules moved to `apps/gmail/chip.ts`. Dead CSS is worse than most dead code because nothing catches it: `lint:css-vars` checks that a `var(--x)` resolves, not that a class is still worn by an element, so these would have sat here looking load-bearing.
96d3f21 to
2ebfc77
Compare
|
All three review comments are addressed in 6535fe8, with replies inline on each thread:
Rebased clean onto dev — 6 commits, MERGEABLE. The one red check — Pixel eval — is the feature working, not a regression. Five surfaces differ, byte-identical before and after the rebase:
Both are the intended change, so the goldens need re-capturing. I can't do it from this branch: Ready for another look otherwise. |
Opening a thread in the Gmail room did not reliably make it the thread the conversation was about. Clicking often did nothing; right-click → Draft a reply did nothing the first time and worked the second; the composer showed two chips naming two different threads; and in Sent the chip named a thread from another mailbox.
Five separate causes, none the same bug.
1. A report the host never heard
report()recorded a snapshot as sent before sending it.send()is fire-and-forget by construction — the only transport is a refused popup at the sentinel — so a report emitted while the host was still subscribing is indistinguishable from a delivered one. Settinglastanyway made that loss permanent for that exact state: the same thread stayed dead forever while a different one worked.resync()drops the memo on a real user action. Not inside our own menu, though — that pointerdown is one step from a click that sends a pointed report, and Chromium spends transient user activation on the firstwindow.open.2. The listener that did not exist yet
The subscription was gated on
live, which resolves from an async availability probe. Until it settled there was no listener at all, while the view was already open and reporting. It now registers ontabIdalone, and the panel will not open until the subscription is up.3. Gmail's cached conversations
querySelector("h2[data-legacy-thread-id]")took the first heading — but Gmail keeps previously-opened conversation views in the DOM, hidden. Measured live: 2 headings present, 1 visible, in a Sent list with nothing open. The stale node does not change as you click around, so the snapshot never changed and the dedupe swallowed every report.openThread()now takes only an on-screen heading and reads the id off it — which also fixes the thread prefetch, sincethreadIdFromHashaccepts hex only and modern Gmail hashes carryFMfcgz…ids it rejects.4. Two chips, two threads
The scope breadcrumb and the promoted canvas selection both drew the thread, under different labels. Right-clicking one row while another is open makes them disagree — the ordinary case. One chip now, reading
Gmail: <subject>, carrying its own ✕, preferring the thread you pointed at."Gmail" rather than "Inbox" is correctness: you move between Inbox, Sent, Drafts and searches, and the report carries no mailbox name.
5. A menu that tore itself down before its own click
blurdoes not bubble, so a capturing listener on window is not "the window lost focus" — it is every element blur in the page. Clicking a menu row moved focus off the Gmail row behind it, that row blurred, and the menu was removed betweenmousedownandmouseup. Noclickever dispatched. The second attempt worked only because focus had already left.Found with the
BRAINS_BROWSER_TRACEdiagnostic added here, after three wrong guesses had each cost a rebuild: the trace showed two passive reports and zero pointed ones, which said the handler was never running.Also here
A refactor that earns its keep.
GmailTabwent 393 → 546 lines carrying this. Rather than shuffle lines under the 400 soft ceiling, the parts that have actually been wrong move out, pure and tested:bridge/report-handler.ts(what a report means),chip.ts(what the chip says it is about),content-port.ts(what a skill can read). None of it was reachable by a test while it lived in an$effectwired to a live WebView; it is now 28 assertions with no DOM, and two mutation checks confirmed they bite. GmailTab is 379 lines and decides nothing.The right-click menu matches the Actions rail — same labels, hints and glyphs. The same command reached two ways should not have two names. The
tagrow is hidden rather than deleted (HIDDEN), since opening a thread now does what it did.Verification
svelte-checknpm testlint:size/lint:imports/lint:css-varscargo clippyledger-net.test.tsfails on Windows only, and it is not a regression. The test builds paths withjoin()(backslashes) and compares against a hardcoded forward-slash list; the two lists are identical in content. It passes on CI's ubuntu runner. Happy to fix it separately — it is a one-line normalisation.cargo clippy/cargo testcould not be run to completion here, becausedevdoes not currently compile on Windows:browser_host.rs:80callsns_window(macOS-only) andlib.rs:67has unused imports. Neither file is in this diff, andbrowser.rsdraws zero clippy mentions. It compiled clean, with all 99 lib tests passing, before the rebase.Checked by hand
Opening a thread as the very first action; switching threads; ✕ returning to inbox-only and staying gone until a different thread; right-click → Draft a reply firing first time; right-clicking a different thread from the one open; Sent naming the right thread; long subjects clipping with the full text on hover.
Not in this PR
The
src/apps/gmail/context.mdskill delta — cross-thread grounding, whose-mailbox, a grounding trace. It is written but has never been exercised against the persona fixture, and does not belong in a PR whose every other line is test-backed. It follows separately, with an eval harness and a synthetic mailbox behind it.🤖 Generated with Claude Code