fix(desktop): align agent message metadata - #6863
Conversation
Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: e8172b5baccd0f4a711f5e19a3bc4313708f1c78..5b6e3737afd680d563de3793eb4871669d09281d (exact head 5b6e3737afd680d563de3793eb4871669d09281d)
Risk: medium — this is a bounded shared metadata/layout change across timeline, system-message, and Inbox renderers, with no relay, IPC, persistence, or identity-contract change.
Blocking finding
-
[P2] The regression test does not cover the claimed Inbox fix and is not deterministic —
desktop/tests/e2e/messaging.spec.ts:393-469. The PR changes the Inbox header alignment atdesktop/src/features/home/ui/InboxMessageRow.tsx:212,239, but the new geometry loop openschannel-generaland measures timeline[data-testid=message-row]/[data-testid=message-timestamp]. It never rendersInboxMessageRowor measures[data-testid=inbox-message-timestamp]. Scoped search underdesktop/tests/e2efound no Inbox agent-owner geometry assertion. Reverting either Inboxitems-baselinerule therefore leaves the committed test green by construction, so one of the PR's stated user-visible fixes is unprotected.The focused exact-head assertion is also unstable: clean, freshly built runs alternated between passing and failing at Smaller because the measured icon-center offset was
1.385040283203125pxagainst a<0.01pxthreshold (messaging.spec.ts:443-467). Independent visible checks found the rendered states sound, which narrows this to the test/layout-readiness contract rather than an established production rendering defect—but a regression gate that alternates red and green on identical source is not trustworthy.
Author action: seed/render an agent message in Inbox; assert author/owner/timestamp geometry and icon centering across Smaller/Default/Larger; wait explicitly for fonts and stable layout before measuring; and mutation-prove that reverting each changed Inbox alignment rule fails the Inbox test. Keep the timeline assertions for the shared component.
Verification owner: author for causal, deterministic regression coverage; CI for repository gates.
Validation
At clean exact head 5b6e3737afd680d563de3793eb4871669d09281d:
- PASS —
just desktop-ci(full Desktop frontend and Tauri package gate). - PASS — independent visible browser captures at Smaller/Default/Larger, 150% zoom, narrow layout, long localization-style names, owner-unavailable state, and accessibility reading order.
- MIXED/FAIL — focused Playwright geometry test: clean exact-head runs alternated pass/fail at the Smaller icon-center assertion.
- CAUSAL FAIL — reverting the Inbox alignment rule did not affect the only new geometry test because that test exercises the timeline renderer.
- GitHub exact-head macOS build, relay E2E, integration E2E, release-candidate, and all four smoke shards were green when reviewed; Desktop Core remained in progress.
Manual/native evidence: browser-visible renderer evidence only; native Tauri capture was not run. This is a confidence gap, not additional author action.
Residual risk: the production CSS appears visually correct in exercised renderer states, but the Inbox-specific contract lacks causal coverage and the new geometry gate can report false failures until layout readiness is stabilized.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: e8172b5baccd0f4a711f5e19a3bc4313708f1c78..235bae0cc2f84b8704db21f1ff1ef908edabddb4 (exact head 235bae0cc2f84b8704db21f1ff1ef908edabddb4)
Risk: medium — user-visible shared metadata geometry and its Desktop regression gate.
Behavior/contracts traced: shared MessageAgentOwner composition in timeline, Inbox, and system rows; Inbox feed-item seeding/rendering; Smaller/Default/Larger geometry assertions; font/layout readiness; browser accessibility reading order; CI selection.
Blocking finding (P2): nondeterministic regression gate. desktop/tests/e2e/messaging.spec.ts:105-230 treats document.fonts.ready plus three identical animation-frame rectangles as proof that the intended webfont/layout is ready. That condition is insufficient. From a clean exact-head E2E build, the focused test passed five isolated launches, but a clean --repeat-each=20 run then passed only two repetitions before repeatedly failing the timeline Smaller icon-center assertion with 1.385040283203125px against the <=0.25px contract (repeats 4–15; later server/harness fallout excluded). Identical source/build bytes therefore produce both green and false-red outcomes.
This also prevents the combined timeline→Inbox test from causally protecting the Inbox fix: reverting only InboxMessageRow.tsx:212 from items-baseline to items-center failed first in the unchanged timeline assertion, so Inbox was never reached. An independently isolated Inbox mutation does fail the Inbox baseline assertion, confirming the production rule matters, but the shipped combined gate can mask that protection behind its flaky first surface.
Author action: make readiness discriminate the intended loaded/rendered font and prove repeated clean launches remain green. Split timeline and Inbox into independent tests (or otherwise ensure one surface cannot prevent the other's assertions), then demonstrate that reverting the Inbox outer baseline rule fails specifically at an Inbox assertion.
Verification owner: author for deterministic, causal regression coverage; :bot: Jude’s code review agent for fresh exact-head rerun; CI for required repository gates.
Validation at matching HEAD:
- PASS: clean
pnpm --dir desktop install --frozen-lockfileandpnpm --dir desktop build:e2e. - PASS: focused geometry test 5 isolated launches in one reviewer lane; PASS 10/10 in an independent lane after a fresh rebuild.
- FAIL: clean focused
--repeat-each=20— 2 passed; repeats 4–15 reproducibly failed the Smaller timeline icon-center assertion at1.385040283203125px; later failures excluded as harness/server fallout. - PASS: independently isolated Inbox outer-baseline mutation failed causally by 1px at the Inbox Smaller managed-by baseline.
- PASS:
pnpm --dir desktop test— 5,556/5,556 in the independent UI lane. - PASS observed at review time: macOS build, Desktop release candidate, relay E2E, and both Desktop integration shards. Remaining Desktop Core/smoke checks were still in progress and do not supersede the reproduced flake.
Manual/browser evidence: browser-rendered timeline and Inbox were exercised at 520px, dark theme, keyboard zoom, and all three font sizes. Accessibility snapshots preserved author → Agent managed by → owner → timestamp → body; decorative icon/middot stayed hidden. Six supplied before/after artifacts were inspected. No native Tauri/WKWebView journey was run.
Non-blocking residual risk: the shared non-membership system-row composition lacks equivalent geometry coverage; localized long-name wrapping and native Tauri AX remain unwitnessed. These are confidence gaps, not additional author-actionable defects.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: e8172b5baccd0f4a711f5e19a3bc4313708f1c78..348cf68cc8968c5c67d39f1671ec886db91d3237 (exact head 348cf68cc8968c5c67d39f1671ec886db91d3237)
Risk: medium — user-visible shared metadata geometry and its Desktop regression gate.
Behavior/contracts traced: shared MessageAgentOwner composition in timeline, Inbox, and non-membership system rows; Inbox feed-item seeding/rendering; Smaller/Default/Larger geometry; intended-font readiness; independent surface causality; wrapping/zoom/browser accessibility; CI selection. No persistence, relay, IPC, identity, or native contract changed.
Findings: no unresolved author-actionable defect. The prior blocker is resolved:
desktop/tests/e2e/messaging.spec.ts:77-153now verifies computedInter Variable, explicitly loads each rendered style/weight/size, checks returnedFontFacefamily/status plusdocument.fonts.check(), waits fordocument.fonts.ready, and only then samples stable geometry.- Timeline and Inbox are independent tests at
desktop/tests/e2e/messaging.spec.ts:638and:665, so one surface cannot mask the other. - Independent exact-head repetition passed 40/40 split tests (20 timeline + 20 Inbox), with the prior
1.385040283203125pxSmaller failure absent. - Causal mutations died on their own surfaces: the timeline alignment mutation failed the timeline Smaller managed-by baseline while Inbox passed; the Inbox alignment mutation failed the Inbox Smaller geometry while timeline passed independently. A separate icon mutation consistently failed timeline icon-center geometry.
Author action: none.
Verification owner: CI for required repository gates; native reviewer only if additional WKWebView/macOS AX confidence is desired.
Validation at matching HEAD:
- PASS: frozen install and fresh
pnpm --dir desktop build:e2ein independent lanes. - PASS: canonical split geometry tests
--repeat-each=20— 40/40 in the product/UI lane. - PASS: an additional lane reached 22 geometry passes before its local Python server terminated; subsequent
ERR_CONNECTION_REFUSEDfailures were harness/server termination, not geometry failures. - PASS: isolated timeline, Inbox, and icon mutation probes produced causal failures, followed by a clean exact-head rebuild and passing canonical tests.
- PASS on prior production-equivalent head and unchanged production UI: full Desktop frontend suite, 5,556/5,556. This changed head modifies only the regression oracle.
- PASS observed at submission: DCO, dead-token guard, release candidate, macOS build, relay E2E, and both Desktop integration shards. Desktop Core/smoke remained in progress; those external gates retain merge ownership.
Manual/browser evidence: narrow 760px, 125% zoom, long Japanese-style author text, owner-unavailable wrapping, and all three font sizes passed on timeline and Inbox. Browser semantic snapshots preserved author → agent/owner → timestamp/body order; decorative bot icon and separator stayed hidden.
Residual risk: native Tauri/WKWebView/macOS AX and system-row-specific geometry were not independently rendered. A trustworthy persisted dark-theme comparison was not produced. These are bounded confidence gaps, not code defects; production theme/UI code is unchanged by the final test-only commit.
Chessing234
left a comment
There was a problem hiding this comment.
the testing here is unusually thorough — waiting on document.fonts.load for the exact computed Inter Variable face before measuring, 40 clean launches, and a mutation check that names the test the reverted rule breaks. that's the right way to make a 1px geometry assertion trustworthy, and i'd be happy to see more of it.
my concern is the hunk the tests can't see.
MessageAgentOwner drops translate-y-[0.125em] from the Bot icon, and with it a comment that was specifically about something baselines don't measure:
self-centerkeeps the icon out of baseline alignment... Centred on the line box the glyph's ink still rides ~1.6px above the text's cap band, reading as a couple of pixels too high; 0.125em drops its optical centre onto that band.
self-center is still there, so the icon is still centred on the line box — the transform that corrected for the glyph's ink sitting high inside that box is what's gone. every new assertion is about the baselines of message-author, message-agent-owner and the timestamp, i.e. the text. the icon's optical position relative to the cap band is exactly the property that isn't asserted, before or after, so the suite would stay green whether or not the icon reads high again.
so: was the earlier reasoning wrong, or is it superseded by the switch to text-message-timestamp? if the line-height change moved the line box such that the correction is no longer needed, that's a great answer and worth one sentence in the pr, because the deleted comment is quite emphatic and the next person to notice a high-looking icon will re-derive it. if it's "we looked and it's fine now", a screenshot of the chip at all three sizes would settle it — the before/after images in the description are the natural place.
the other two hunks look right to me. items-center → items-baseline on the Inbox message-header is the same rule the Timeline header already used, and swapping text-xs leading-4 for text-message-timestamp is what makes the owner chip track the timestamp at every font size instead of at one.
Summary
managed by, owner, and timestamp share a baseline at every message font sizeInter VariablefontTesting
pnpm --dir desktop test(5,556 passed)pnpm --dir desktop checkpnpm --dir desktop build:e2eInbox managedBy baseline at smaller font sizeby 1pxBefore / After
Agent message header (
alice· managed bybob· timestamp) captured from the mock-bridge E2E build at 3× so per-pixel baseline drift is legible. BEFORE isorigin/main(e8172b5, the PR's merge base); AFTER is the production state introduced at235bae0cc. Current head348cf68ccchanges only the regression test, so the rendered production state below is unchanged. Each pair covers both surfaces the change touches — the timeline row and the Inbox detail header — at all three message font sizes. The thin 50%-alpha cyan guide follows thealicetext baseline in each panel.Timeline
Inbox
In each BEFORE frame the bot glyph rides above the name's baseline and the
managed by/timestamp run sits slightly high; AFTER, the icon is centered on the metadata line box and the author,managed by, owner, and timestamp share one baseline.