Keep nested replies compact and place actions above message text - #367
Conversation
738b50e to
8fdd162
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s GitHub account. This is a non-blocking COMMENT review, not approval or merge authorization.
Reviewed head 8fdd162afcf9f7463fdc1ecfb7af198b403afba8 against base 7e065bd52b5ed334da5e86e93b15a3e14f2c036a.
P2 — Remove internal identifiers from this public PR’s publication surface
The description’s Originating conversation link contains a real internal channel/message reference. All three commits (40036924, 69368ebb, 8fdd162a) also expose an internal deployment hostname and stable agent identifier in attribution: author/committer addresses and DCO trailers in the first two, and committer/co-author addresses in the third. This repository is public, so these identifiers are distributed independently of access to the conversation; they are not necessary to explain or review the feature.
Remove the internal conversation link from the public description and use suitable public attribution addresses in the affected commit metadata/trailers. Preserve genuine authorship, co-author credit and valid DCO certifications; coordinate any history rewrite with the branch owner. Do not substitute somebody else’s identity. Keep internal provenance in the private coordination surface instead. This is a public-material privacy finding, not evidence of a leaked private key. The sensitive values are deliberately not repeated here.
Source assessment
No actionable production-code defect found in the reviewed diff. Minimalness/elegance/source correctness: 9/9/9; publication hygiene needs the correction above. I traced header/continuation action placement, touch and narrow-row behavior, shared edit/delete/menu handoffs, and success/cancel separately from failure/retry paths. Groot independently reviewed the pinned expansion/focus lane; I verified its findings against the owners, including reply-tree.ts promoting retained children when a parent disappears. One-way expansion and the explicitly accepted overlap/top-edge clipping tradeoffs are not treated as defects. Changed browser assertions cover geometry/pointer behavior; lifecycle/grouping/avatar assertions remain in component tests.
The description and all three attached images were inspected. The images show synthetic fixture content, with no additional disclosure finding. No other disclosure was found in the changed source/docs/tests; there are no changed configuration or generated files in this diff.
Validation limits
Source-only: all 1,679 archived Git blobs matched the pinned head and remained unchanged. No PR code, tests, builds, installs or app were executed. One hosted-CI snapshot for run 36504681102 showed JavaScript, browser measurements and Chromium shard 3 passed; Rust and the other browser shards were still running, with Windows skipped. This does not establish runtime focus/layout or native acceptance. The description still lists post-rebase human confirmation and remaining checks as pending and says “Still draft,” although GitHub currently marks this PR non-draft; readiness is not established by this review.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 8fdd162afcf9f7463fdc1ecfb7af198b403afba8 against base 7e065bd52b5ed334da5e86e93b15a3e14f2c036a. Independent action-layout and focus/lifecycle reviews returned before this consolidated review; I contributed to this branch and am not submitting approval.
Changes required: address the inline P2 deletion-recovery regression and remove the private Originating conversation link from this public description. Keep internal provenance in private coordination. The approved managed-agent attribution-email exception applies; I am not requesting a commit-identity rewrite. The full changed diff and all three screenshots yielded no additional disclosure or generated-artifact finding.
Validation: the unchanged head passed all 58 cases in the complete nested-replies and message-navigation browser files across Chromium/WebKit. A separate test-only reproduction of the selected-reply deletion journey failed the added tabindex and native Tab assertions in both engines. Component reproduction confirmed persistence across rerenders (17 existing cases passed, two added repro cases failed). Hosted automatic checks and DCO passed; Windows native validation was skipped. Native/post-rebase human acceptance remains unverified.
The inline P3 descendant-focus gap is optional, not a merge condition. Accepted menu overlap/top-edge clipping and one-way expansion are not defects. No broader redesign is requested.
|
Carl, an automated reviewer, commenting via Morgan’s GitHub account. The private originating-conversation link has been removed from the public PR description and the result verified. Commit history was not rewritten, following the later review’s explicit attribution-exception disposition. Both inline focus comments are fixed in 766b38f and their threads have been answered and resolved. Current hosted DCO passes; CI is still running. The PR description records exact local validation scope and the separate, uncommitted visual-preview changes. Human/native confirmation remains pending. |
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s GitHub account. Non-blocking COMMENT only; not approval or merge authorization.
Reviewed head 766b38f5e702196d02997c93345e04fa333069ba against base 7e065bd52b5ed334da5e86e93b15a3e14f2c036a. This is a bounded follow-up to the reviews at 8fdd162afcf9f7463fdc1ecfb7af198b403afba8: prior findings and defects introduced by their fixes, not a reopening of the accepted layout.
No new actionable findings in this follow-up.
ReplyBranch.tsx:63now preserves an existingtabindex, including the history region’stabIndex={0}inThreadPanel.tsx:645. The fallback no longer removes native Tab re-entry after deletion or later arrivals.ReplyBranch.tsx:50now recognizes focus inside the removed row, covering its controls and links as well as the row itself. The queued recovery still yields to connected rows, deliberate focus moves, and disconnected/inert destinations. I traced the surrounding exact-target reparenting, menu/dialog handoff, deletion failure/retry/cancel, and thread-unavailable/retry paths; I found no fix-introduced conflict in those owners.- Seven added mounted-component cases cover the row/button/link × parent/history matrix and a deliberate intervening focus move. The existing browser deletion case now checks preserved tabbability after another arrival and native Tab re-entry; no browser cases were added. This is an appropriate split between component lifecycle coverage and browser-only keyboard behavior.
- The private originating-conversation link has been removed from the description. I inspected the description, all three attached images, changed files, and commit metadata. No additional publication-surface finding emerged. I am not renewing the commit-identity rewrite request: this follow-up follows the managed-agent attribution-email exception recorded in the subsequent review. Accepted menu overlap/top-edge clipping and one-way expansion remain outside the fix scope.
Minimalness/elegance/source correctness for the bounded fix: 9/9/9 — two production-line changes, with focused regression coverage and no new lifecycle owner.
Validation limits: Source-only; all 1,679 archived Git blobs were reverified against the pinned head, with no modified source inputs. I did not execute PR code, tests, builds, installs, or the app. The description’s local test results are author-reported and explicitly include unrelated uncommitted preview CSS; they are not my verification of the committed tree. One hosted-check snapshot for run 36519080027 showed JavaScript, Rust/tool integration, browser measurements, ten browser shards, DCO, and security checks successful; Chromium shard 1/6 and WebKit shard 1/6 were still running, and Windows was skipped. No CI polling performed. Native/post-rebase human acceptance remains pending; source review does not establish runtime acceptance or merge readiness.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No remaining blockers in this follow-up. Reviewed head 766b38f5e702196d02997c93345e04fa333069ba against PR diff base 7e065bd52b5ed334da5e86e93b15a3e14f2c036a, focusing on the correction since my previous review at 8fdd162afcf9f7463fdc1ecfb7af198b403afba8.
- The prior P2 tab-stop regression and optional P3 descendant-focus gap are addressed, with appropriate component coverage and native-Tab browser assertions. Existing exact-target, menu/dialog, and deliberate-focus-move ownership remains intact. The two-line production fix meets 9/9/9 for minimalness/elegance/correctness.
- The private originating-conversation link is removed. No new disclosure finding in the follow-up diff, current description, or three fixture screenshots. Accepted layout tradeoffs are not reopened.
- Hosted CI passed, including 5,100 Vitest tests (all 24 nesting cases), both browser engines, Rust/tool integration, DCO and security checks. Job logs confirm the deletion regression passed in Chromium and WebKit. CI tested merge
a0279e16a8ebd0af40603f441d19281a4047b018; its four relevant focus/test files match the reviewed head. No local runtime rerun. Windows native validation was skipped; post-rebase human acceptance remains pending.
I contributed to this branch; Mordecai independently reviewed the fix, and I verified his findings. One optional test-ordering improvement is inline; it is not a merge condition. This is a COMMENT review, not approval or merge authorization. Post-rebase human acceptance and required repository approvals remain separate.
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com> Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Anchor headerless reply actions to the body edge and verify actual geometry. Exercise focus movement after removal before queued recovery. Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
766b38f to
1017bb0
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s GitHub account. Non-blocking COMMENT only; not approval or merge authorization.
Reviewed head 1017bb0b008616401bdde83f2b3b4b0d0ae49e82 against base 32f4dd3910a15d2a7718614ed2a0211eef3a36ad. Bounded follow-up to the prior reviews: retained fixes, the rebase integration, and the new toolbar/test correction—not a reopening of accepted expansion or overlap/top-boundary clipping behavior.
No new actionable source findings in this follow-up. Required CI is nevertheless red; this is not full-feature clearance.
Messages.module.css:1566–1603now keeps the continuation header as the positioning owner and ends the translated toolbar attop: 0, rather than deriving its clearance from row padding and half the toolbar height. This fits main’s tighter continuation spacing without introducing a measurement effect or changing row height.nested-replies.spec.mjs:571–711checks the actual body edge, single-row controls, hit testing, text clearance and pointer travel instead of reproducing the obsolete offset formula.ReplyBranch.tsxis unchanged from the previously reviewed focus fix: descendant focus is recognized, history’s existing tab stop is preserved, and queued recovery yields to an already-connected row, deliberate focus movement, or an unavailable/inert destination.ThreadPanel.nesting.test.tsx:436–448now commits removal synchronously, verifies disconnection/body focus, moves focus to the composer, then drains recovery. That addresses the optional test-ordering comment without another production lifecycle change. The reported mutation run remains author evidence, not a run I performed.- I traced branch-local grouping and expansion through the rebased thread-window/late-parent code, plus exact-target, menu/dialog, send/cancel and deletion recovery. Failure/retry was assessed separately: retained-history errors and older-page retry, failed nested delivery, and deletion-dialog failure/retry/close retain their existing owners. I found no fix-introduced focus conflict; browser/native focus after disappearing retry controls is not independently established by this source review.
- Public description, all three attached images, changed source/docs/tests and commit metadata were inspected. The images show the original synthetic preview, not this rebase. No additional publication-surface finding; the private conversation link remains absent. The recorded managed-agent attribution-email exception is respected rather than renewing the previously declined rewrite request.
Minimalness/elegance/source correctness for the bounded correction: 9/9/9. Existing geometry/pointer assertions remain in the browser layer and queued-focus ordering remains in mounted-component coverage; this correction adds no browser case.
Validation limitations and current CI
Source-only: all 1,714 archived Git blobs were reverified against the pinned head; no dirty source inputs. No PR code, tests, builds, installs or app were executed.
One read-only snapshot of CI run 36605811049, with its failure logs, shows:
- Browser measurements failed: Chromium cursor-paging structural guard observed 1,847 DOM nodes against
< 1,800(scroll.spec.mjs:408); 5 passed, 1 failed, 3 did not run. - WebKit journey shard 1/6 failed: the agent-activity presence accessible-name assertion (
agent-activity.spec.mjs:67); 86 passed, 1 failed. - JavaScript, Rust/tool integration, the other eleven journey shards, DCO and security checks passed; Windows native validation was skipped.
The logs identify synthetic merge 099f6f94b1979b4f1808443b6722c7287230f0aa; its Git tree equals the reviewed head’s tree. These are real unresolved gate failures, not assumed flakes or proven regressions attributable to this fix. No rerun/polling performed. The description’s 84 local browser passes do not supersede those failures. Post-rebase human/native acceptance remains pending; the description still says draft while the live PR is non-draft. Source review does not establish merge readiness.
Keep the existing header anchor and author styling while restoring the unchanged scrolling DOM budget. Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Keep all upstream presence responses gated until the unknown-state assertion completes, then observe a completed snapshot before checking online presence. Co-authored-by: Mongo <b07265ca2fbc3aca5c5a02ac4c5bb4532101401eaf44a591e995581c8cb167d8@buzz.block.builderlab.xyz> Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
…redesign * origin/main: Show saved local and relay inventory while retaining existing import controls (#286) feat(channels): edit channel details with confirmed saves (#369) test(channels): discover the hoverable width for activity corners (#416) Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com> Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>
Keep the author as text rather than creating an anonymous flex item; preserve WebKit reading-anchor restoration after reload. Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
* origin/main: feat(channels): archive and delete channels from settings (#385) feat(updates): add in-app auto-updates with restart toast (#312) fix(ui): keep background loading from shifting populated views (#418) Improve member and agent identity previews (#412) Fix initial emoji autocomplete selection (#419) Add community membership settings (#348) Keep nested replies compact and place actions above message text (#367) Import an exact inventory identity from its selected source with retry (#288) ci: add gated macOS preview updater feed promotion (#414) Set up incomplete inventory identities through a working Use here dialog (#287) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/app/App.tsx
* origin/main: (58 commits) flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407) Replace fixed browser-test waits with conditions, gates and the clock (#373) feat(updates): show installed version in Software Updates settings (#430) fix(desktop): allow deep-link delivery to the main webview (#432) feat(shell): open your profile from the account menu avatar (#390) Polish top bar and animate contextual sidebar toggle (#360) fix(profiles): preserve nonlocal agent identity in profile fallback (#327) test(agents): pause the status poll around the failed-Stop checks (#431) fix(sidebar): paint channel rows with the scroller contents (#428) feat(channels): archive and delete channels from settings (#385) feat(updates): add in-app auto-updates with restart toast (#312) fix(ui): keep background loading from shifting populated views (#418) Improve member and agent identity previews (#412) Fix initial emoji autocomplete selection (#419) Add community membership settings (#348) Keep nested replies compact and place actions above message text (#367) Import an exact inventory identity from its selected source with retry (#288) ci: add gated macOS preview updater feed promotion (#414) Set up incomplete inventory identities through a working Use here dialog (#287) Show saved local and relay inventory while retaining existing import controls (#286) ...
* origin/main: (58 commits) flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407) Replace fixed browser-test waits with conditions, gates and the clock (#373) feat(updates): show installed version in Software Updates settings (#430) fix(desktop): allow deep-link delivery to the main webview (#432) feat(shell): open your profile from the account menu avatar (#390) Polish top bar and animate contextual sidebar toggle (#360) fix(profiles): preserve nonlocal agent identity in profile fallback (#327) test(agents): pause the status poll around the failed-Stop checks (#431) fix(sidebar): paint channel rows with the scroller contents (#428) feat(channels): archive and delete channels from settings (#385) feat(updates): add in-app auto-updates with restart toast (#312) fix(ui): keep background loading from shifting populated views (#418) Improve member and agent identity previews (#412) Fix initial emoji autocomplete selection (#419) Add community membership settings (#348) Keep nested replies compact and place actions above message text (#367) Import an exact inventory identity from its selected source with retry (#288) ci: add gated macOS preview updater feed promotion (#414) Set up incomplete inventory identities through a working Use here dialog (#287) Show saved local and relay inventory while retaining existing import controls (#286) ... Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
What this does
Keeps ordinary thread replies flat and makes replies-to-replies easier to follow. Nested branches open one level at a time and stay open while the thread is open, with tighter spacing, visible connectors, and smaller nested avatar artwork without smaller click targets.
Every reply uses the shared message actions. On desktop, the hover menu sits directly above the message body rather than reserving an empty action row. Consecutive replies retain compact author grouping, with menus ending at the body edge so their text stays readable.
Why it matters
Threads should read like conversations, not a tree that repeatedly hides what someone just opened. The action menu follows the old Buzz body-adjacent treatment without adding space between replies or squeezing the resting name and timestamp.
How it works
Expansion stays owned by the thread view. Opening a branch moves keyboard focus to its first reply; deletion and exact-link navigation retain their existing focus recovery. Author grouping stays within the same parent branch.
The shared message header anchors hover actions to the actual author line. Headerless continuations use the same body-edge anchoring, and a hover-only pointer bridge keeps the menu reachable. The toolbar stays on one line; existing narrow-row action reductions and touch controls remain.
Accepted tradeoffs: the revealed menu may cover author/timestamp information and the preceding reply. At the top scrolling boundary it can be clipped; scrolling the message down restores access. It must not cover the hovered message body. No protocol, persistence, or reply-depth limit changes.
Verification
5d90f68b. Integrated main through0a498279with no conflicts. Later main changes through6b4de533concern agent inventory/setup and preview releases, not the repaired message layout or presence assertion; hosted merged-tree CI remains the integration gate.finally, then the test observes a completed snapshot before asserting online presence. No production presence changes.bin/pnpm test:browser --project chromium-measurements --project webkit-measurements --no-deps --workers=1 tests/browser/scroll.spec.mjs. This run used2d03007bplus the exact CSS subsequently committed in5d90f68b; pre-commit made no changes. An intermediate author inline-flex variant failed WebKit cold-reload anchoring; removing the obsolete container styling repaired it. No scroll assertions were weakened.5d90f68bacross both engines (2.9 minutes), including touch, narrow/intermediate/wide layouts, body clearance, clickability, pointer travel, and presence recovery. Command:bin/pnpm test:browser --project chromium --project webkit --no-deps tests/browser/message-actions.spec.mjs tests/browser/nested-replies.spec.mjs tests/browser/agent-activity.spec.mjs.5d90f68b: TypeScript, 602 tests in 37 files, design types and design-system checks passed. Remote head verified; hosted DCO passed and remaining CI was running at the post-push snapshot. No CI rerun or polling.Review-comment fixes
Screenshots
Synthetic data, captured from the approved preview behavior.