Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two P2 regressions, detailed inline. Merge criteria: preserve the exact target’s focus when delayed ancestry changes its DOM placement (without reclaiming deliberately moved focus), and fully hide retained conversation contents while Channel Settings is displayed.
Reviewed head 9d5fc9b831a2db41defd28fd7eaa5125b55d6ea3 against base/merge-base 8842b3ac05862e069ab0adf2f30e11a3af084042. Integrated independent protocol/count and presentation reviews; no additional blocker found in those inspected paths. Approved spacing and controls-row decisions are unchanged.
Validation: source/contract review plus temporary, fixture-only Playwright probes against that head with no production edits. Both regressions reproduced in Chromium and WebKit; the two moved-focus controls passed. Existing delayed-context coverage uses direct replies, not a nested target whose parent arrives later. Broad suites were not duplicated; live relay mutations, packaged native behavior and physical touch were not exercised.
CI/mergeability, checked at review closeout: all automatic hosted lanes and CI required pass; GitHub reports mergeable. The optional Windows lane was skipped. These are separate from the reproduced defects above.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source re-review clear at 38f86ac27d8414bfc236ed72859d43cac862a150 against 8842b3ac05862e069ab0adf2f30e11a3af084042: both prior P2 findings are source-fixed; no new actionable defect found. COMMENT only, not approval or merge readiness.
- Focus transfer is limited to focus owned at selected-row detach, bounded to the synchronous ancestry update, with moved-focus, aborted-navigation and inert guards. Retained wrapper opacity prevents revealed spoilers painting through Settings without discarding layout/state. The targeted regression sources include nested DOM replacement, deliberately moved focus, later collapse/reopen and spoiler restoration.
- Crowded checks retain all three widths and their interaction/geometry assertions. Isolating fixtures adds two cases per engine without changing the timeout or retries; the separate journey still exercises same-session resizing. Hosted runtime cost remains unmeasured.
- No CI workflow ran on this head. Only Semgrep, zizmor and DCO passed. The previous WebKit timeout belongs to
282abdde, not this tip. GitHub reports conflicts with main: resolve them and validate the resulting head, including Chromium/WebKit repairs and isolated crowded cases, before merging.
This review used source/contract analysis and existing hosted metadata only; no PR code or tests were executed. Live relay mutations, packaged native behavior, physical touch and a complete keyboard/screen-reader walk remain unverified. Approved controls spacing is unchanged and not a blocker.
wesbillman
left a comment
There was a problem hiding this comment.
Pinky, an automated reviewer, commenting on Wes's behalf. This review also includes Brain's thread review.
Two P2 regressions need fixing at 0640e5813702c9c3a874e66c7b1ef3c2a5865e62; details are inline. To merge:
- A reply's message subtree stays stable when it goes from zero to one child, or from one to zero.
- Outside Settings, a thread or linked panel stays stacked with a side channel panel instead of overlapping it.
Each fix needs regression coverage.
Checked and found no problems:
- Nested reply signing adds a
roottag only when the parent isn't the root, which matches the relay's ancestry validation. - The fold and row identity carry
replyParentId. - Timeline totals use the relay's
descendant_countand fall back to a validatedreply_count. - The merge resolution of the traffic integration test keeps main's assertions.
Hosted CI is green at this head: 3,340 Vitest tests, all Chromium/WebKit journeys including nested-replies, browser measurements, and Rust. Windows native validation was skipped.
Both findings come from reading the source; neither was reproduced in a browser, and no local suites were run. This is a COMMENT review, not an approval.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 0640e5813702c9c3a874e66c7b1ef3c2a5865e62 against 119195ea331de33c8480bab180df0091ca8e9421: both existing P2 findings remain actionable. I independently traced the owning code and callers; details and regression expectations are already in the linked inline threads:
- Preserve the message subtree across zero↔one-child transitions. The keyed outer
liand its memoized ref remain mounted while the inner row is replaced, soselectedBranchRefdoes not run; the completed one-shot reveal does not repair lost focus. Preserve local row state as well, rather than adding repeated focus grabs. - Restore side-panel stacking outside Settings. Limit the shared grid cell to Settings retention; preserve the existing thread/link + side-panel arrangement rather than closing a user's thread as a workaround.
Exit criteria: fix both transitions and add regression coverage, retaining the prior late-ancestry focus, moved-focus, Settings paint/inertness and crowded-layout protections.
Exact-head hosted CI passed on the merge with the stated base: 3,340 Vitest tests and all Chromium/WebKit shards. Those passes do not cover these two missing transitions. Windows native validation was skipped. This was source-only review; neither finding was reproduced in a browser here, and packaged-native/deployed-relay behavior remains unverified. Approved permanent controls spacing is unchanged and not a blocker.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review clear at f605f7571c2ab5afe95db32dee3a2f3e1b7d6441 against 6fa0e9a63690e7eab413f9e5c11be7a05a4f6c4f. Both prior P2 findings are repaired; no new actionable defect found in the changed paths. COMMENT only, not approval or merge readiness.
- Reply rows now retain the same
ReplyBranch/MessageRowcomposition through first-child arrival and last-child deletion. Regression sources check node identity, revealed spoilers, owned focus and deliberately moved focus. The final helper-only commit restores shallow traversal of the render callback without removing assertions. - Shared-grid overlap is restricted to Settings and inert retained panels. The new Todos journey checks thread/link stacking in both opening orders; existing Settings/Bestie geometry, late-ancestry focus, Settings spoiler-paint and nested-collapse protections remain intact. Approved controls spacing is unchanged.
- Hosted CI run 36078744083 passed, including 3,553 Vitest tests, Rust/Node integration, browser measurements and all Chromium/WebKit journey shards. The JS checkout log verifies merge
c5beddd75d1c349bd30d824f3ef3f10253631638of this head into the stated base. Windows native validation was skipped.
Validation was source-only on Blox plus existing CI evidence. This invocation executed no PR code/tests and did not exercise deployed-relay, packaged-native, physical-touch or full screen-reader journeys. Other jobs’ checkouts were not individually audited; normal human review/branch protections remain.
Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
…s changes Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Co-authored-by: Mordecai <6c809112bf9d6e023eceba705322b380271cde09c3f8b21514a1349ed69207c7@buzz.block.builderlab.xyz> Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
f605f75 to
8e44612
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source re-review clear at 8e44612b44e9510c9a0265c9e4760756a2d53d37 against 30c3589ba57ce926ac19c5526d8b87cde35d6e48. No actionable regression found in the rebase from the previously reviewed head. The stable reply-row composition, Settings/panel stacking fixes, exact-target focus behavior and narrow-panel rules are preserved; the author/status integration retains continuation timestamps.
Required CI is still red, so this is not merge-ready certification. Run 36087531104 exercised synthetic merge 4593bdd25d01781f5a0fc546e382e04fabe66382 of these exact pins. All four Chromium/WebKit journey shards passed, including the affected nesting/navigation/layout coverage. JavaScript finished with 3,647 passing tests and one failure in AgentModelPicker.test.tsx (the “Pi discovers extension providers…” case cannot find the accessible “Cancel model lookup” button). No causal regression from this PR was established; resolve that required gate before merge.
Source-only review on isolated Blox plus existing hosted CI evidence. No PR code executed or CI rerun by this review; Windows native validation was skipped and live/packaged-native acceptance is not established. This is a COMMENT, not approval.
What this does
Reply to a specific message inside a thread, then expand or collapse its conversation branch. The composer shows who you are replying to without discarding your draft. Sending opens the branch containing your reply. Sessions stay inline; unrelated legacy thread features are out of scope.
Behavior and implementation
Validation
Current head:
8e44612b44e9510c9a0265c9e4760756a2d53d37, rebased onto main30c3589b. Local, remote branch and PR heads match; working tree is clean. Hosted CI and reviewer re-review remain gates.Main rebase (
8e44612b)0640e581, which ordinary rebase does not replay. Compared the entire resulting tree with the expected merge of the old head and new main: only the deliberately resolved MessageRow conflict differs. Independent exact-head source review found no preservation blocker.8e44612b, completelayout,message-navigation,nested-replies, anduser-statusbrowser files passed 78/78, Chromium/WebKit, two workers, 3.0 minutes. Isolated output:test-results/main-rebase. No new or removed browser cases in this rebase. This is not the full browser suite; fixtures exercise production app/broker with modeled upstream, not deployed-community or packaged-native behavior.Earlier validation below belongs to its named pre-rebase snapshots.
Stable reply rows and side-panel stacking (
f605f757)6d32dc05, completelayout,message-navigation, andnested-repliesbrowser files passed 76/76, Chromium/WebKit, about 2.8 minutes, isolated outputtest-results/integrated-main-review. This is an affected-file run, not the full browser suite. Finalf605f757changes only an existing unit-test traversal helper; production/browser-test content is identical to the integrated run.f605f757: 122 test files / 1,856 related tests, TypeScript, and all design-system type/color/contrast/foundation/icon/adoption guards. No hook bypass. Hosted DCO passed; remaining hosted checks were pending at readback.Persistent-sidebar integration (
0640e581)nested-replies.spec.mjs: 14/14 passed, Chromium/WebKit, two workers, 1.3 minutes. Includes new 390px crowded-branch case per engine (one net case added per engine, none removed), all 12 collapse/reopen/hit/focus cycles, touch no-overflow, deep-row readability, and threshold-adjacent widths. Browser-only justification: actual layout, hit testing, focus, and pointer media behavior. Existing desktop assertions remain; mobile actions are correctly asserted visible rather than hidden.layout,message-actions,message-navigation, andthread-unreadfiles: 70/70 passed, both engines, two workers, 2.0 minutes. Both browser runs used the resolved working tree based on38f86ac2plus main119195ea, committed here without formatter changes. A preceding diagnostic WebKit run once failed the existing Up-to-edit focus step; subsequent full-file runs passed without a related code/test change. No claim that this intermittent failure is eliminated.0640e581: 119 Vitest files / 1,786 tests passed, TypeScript, design types/guards, formatting/lint. All 15 PR commits have signoffs; hosted DCO passed. Local policy-relay/browser evidence only, not packaged-native verification.Crowded-thread CI test isolation (
38f86ac2)282abddeexceeded the single 120-second deadline in the crowded-controls test. Its trace showed 33/36 completed branch cycles, 239 clicks and 430 attribute reads; completed individual actions stayed under one second. All other automatic lanes passed; this shard had 149 other passing tests.nested-replies.spec.mjspassed 12/12, Chromium/WebKit, two workers, in 1.0 minute on the pre-format working tree based on282abddecommitted here. Each isolated crowded case took 9.6–12.8 seconds locally. Earlier unsplit WebKit case took 47.8 seconds in the local two-file run; hosted exhaustion is not a directly comparable local performance measurement. Additional per-width fixture setup is intentional; hosted new-head timings remain pending.38f86ac2: TypeScript, 104 Vitest files / 1,587 tests, design types/guards and staged formatting/lint. New-head CI and reviewer re-review remain pending. GitHub readback at repair start exposed only the two original review comments, with no newer inline or issue comments.Late ancestry focus and retained spoiler repair (
282abdde)message-navigationandnested-repliesfiles passed 44/44 in Chromium/WebKit, two workers, about 2.4 minutes, on the pre-format working tree based on9d5fc9b8committed here. Includes programmatic collapse/reopen without stale focus restore. Independent bounded source review found no actionable defect; reviewer did not run tests.282abdde: TypeScript, 104 Vitest files / 1,587 tests, design types/guards and staged formatting/lint. First push attempt encountered an existing MessageActionBar keyboard-focus test failure; an unchanged second full hook run passed. No test bypass, timeout change or assertion relaxation.Timeline count repair (
9d5fc9b8)descendant_countover direct-onlyreply_countfor the existing presentation total. Valid zero wins; absent/invalid totals retain the validated direct-count fallback. No UI, layout, persistence, or relay changes.block/buzzchannel-window bridge and live summary emitter. Signed fixture coverage checks one direct reply with three total, live increases/decreases, stale and spoofed summaries, and zero. Render coverage checks visible and accessible badge text. Five assertions failed before the correction; all 140 tests in the three affected files passed afterward.9d5fc9b8: TypeScript, 104 Vitest files / 1,587 tests, design types/guards, formatting/lint and secret scan. Independent bounded source review found no actionable defect; reviewer did not run tests.Earlier nesting and settings repair (
e35f02b7)e35f02b7. This includes all 10 nesting component tests and the regression preserving collapse preference across temporary root loss. Mandatory commit hooks passed for the repair and merge. All PR commits have Signed-off-by trailers.e35f02b7, Chromium and WebKit, two workers: completemessage-navigationandagent-activityfiles (about 1.4 minutes locally). These exercise exact reveal, retained target identity/focus, reading dwell, settings isolation, restored navigation, and main's new activity preview.9437955b: completechannel-settings,completion-layout,layout,message-navigation,navigation-thread-history,nested-replies,thread-unread,messages, andtypingfiles, both engines, two workers (about 3.5 minutes locally). This is not claimed as an 86-case run at the integrated head.nested-replies,navigation-thread-history,thread-unread, andmessage-editfiles passed 20/20 in both engines on6e0bb292plus the test-only publication wait committed as9437955b. The nested journey covers composer → broker signing → policy relay → delivered row, rejected send/retry with signed ancestry, held pending sends, selected-parent editing/cancel/native undo, collapse, deep links, missing parents, and responsive layouts in both themes.Repair of the six previous hosted browser failures
9437955b..c33122c1, not runtime or the merge. Integrated-head inspection confirms the merge did not change the repaired panel layout or root-branch files; integrated runtime scope is the 40-case run above.All automatic hosted CI lanes passed at
e35f02b7, including all four browser shards and CI required. That pass does not cover the subsequent count repair; new-head CI is a separate gate.Scope and remaining checks
Diff size at
f605f757against integrated main: tests +1,782/−127 across 19 files; implementation +1,014/−196 across 17 files; docs +17/−11 across 1 file. Tests are counted separately from implementation.Current narrow-panel screenshots
Synthetic fixture content from the successful final browser run, not a deployed conversation. A 1024-wide desktop window yields a roughly 311-wide thread panel. Indentation stops growing to preserve reading space. Controls reserve space even while hidden.
Crowded sibling/continuation example, showing the intentionally reserved controls spacing: