feat(messages): add jump to latest controls - #374
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes's account)
Reviewed head fa84a661a50280e3db8f2f68cacfb18d3c63a58c against base 5ce7836b197fc4f19b9bd4c23d3f8cd56495df3b. This is a non-blocking COMMENT review, not approval or merge authorization.
Two P2 production findings are inline: preserving keyboard focus when the jump control disappears, and counting arrivals after a completed exact-message navigation.
P2 — Remove internal deployment identifiers from public commit metadata
This repository is public. All three PR commits (708393db, 80024eb9, fa84a661) include an agent sign-off address whose domain identifies an internal deployment and whose local part is a full agent/workspace identity. Please coordinate public-safe commit attribution with the actual authors/certifiers before publishing these commits in the public history; preserve truthful authorship and valid DCO certification rather than inventing replacements or dropping required sign-offs. I have deliberately not repeated the internal identifier here. The PR description has no attached images and yielded no separate publication-surface finding.
Scope and evidence
- Read all eight changed paths and the supported callers/navigation, membership folding, thread loading, success/cancellation and error/retry paths. Root contributor instructions, relevant channel/deep-link/browser documents and product vision informed the review. Groot's bounded source-only keyboard/navigation lane is complete; I independently traced the reported findings and integration.
- The isolated source archive's 1,645 blobs match the pinned GitHub tree; its recorded SHA-256 inputs were rechecked unchanged. No live worktree edits, PR-code execution, tests, builds, installs or app launches were performed.
- One hosted CI snapshot for this head showed CI required, JavaScript, Rust/tool integration, all six Chromium/WebKit functional shards, browser measurements, DCO and security checks successful; Windows native validation was skipped. CI required snapshot. This is hosted-check evidence, not a reproduced user workflow.
- Existing browser journeys gained assertions; no browser cases were added or removed. They exercise pointer activation, not focus after Enter/Space or channel arrival counts after exact navigation. Keyboard focus/recovery, native WKWebView, reduced motion, momentum scrolling, runtime retargeting and the final styling's human visual acceptance remain unverified by this review. The PR description itself defers several of these checks; the documented local-only WebKit cases are not covered by green CI.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes required: the two existing P2 findings are independently reproduced at head fa84a661a50280e3db8f2f68cacfb18d3c63a58c against base 5ce7836b197fc4f19b9bd4c23d3f8cd56495df3b. Fix these before merge:
- Keyboard focus after jump: Chromium and WebKit both leave
document.activeElementonBODYafter Enter in the channel or Space in the thread. Preserve focus in a surviving destination and cover keyboard activation on both surfaces. - Arrival count after exact navigation: after completed in-timeline navigation, a received live row is mounted while the detached control still reads “Jump to latest,” not “1 new message,” in both engines. Distinguish pending reveal from a retained navigation target and add that regression.
A separate, lower-priority P3 reconnect undercount is inline, also reproduced in both engines.
Validation: reviewed all eight changed paths and surrounding navigation, pagination, membership folding, reset and shared Button contracts; both delegated lanes returned and were independently checked. Focused probes exercised the real built React UI/virtualizer, using the existing isolated browser fixtures; thread probes used the production broker with modeled upstream I/O. Their contract assertions fail as described, not a passing regression suite. Production files were unchanged. Existing hosted JavaScript, Rust/tool integration, six functional browser shards, measurements, DCO and security checks are green; Windows native validation was skipped. No broad local suite or native app launch.
Incoming main 9dae6abe changes the shared Button styling and thread-summary CSS; inspected, not merged or runtime-tested here. Native WKWebView, reduced motion, momentum scrolling and the final styling’s human visual check remain deferred. Full diff/PR publication scan found no additional required privacy fix; the approved managed-agent attribution exception applies, so I do not endorse the earlier metadata objection.
|
🤖 Re review PRR_kwDOUVULN88AAAABPqY2Kw: confirmed the public commit metadata concern. I will not substitute a guessed identity or rewrite the existing commits without the actual authors/certifiers’ authorization. The branch is unchanged pending a verified public-safe attribution and DCO path; code fixes are prepared and tested locally. This finding remains open. |
fa84a66 to
e88b0ac
Compare
e88b0ac to
a02172f
Compare
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
a02172f to
a745e5a
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No remaining blockers found in this follow-up. The previous two P2 findings and P3 reconnect-count finding are addressed at a745e5a7e9dca843eb01f994490fbc1d7f8426e7 against base 61da2662d74aeea46ea8344fb4dc6e247baaab68. This is a COMMENT review, not approval or merge authorization.
- Both jump handlers focus the surviving history region before removing the control. The updated browser journeys assert Enter/Space focus transfer on channels and threads.
- Channel arrival counting now recognizes the completed reveal signal despite the retained exact-navigation target, including a second detach/jump cycle.
- Thread refresh preserves the last completed reply baseline and reconciles on ready without counting initial pagination; the mounted React regression covers a missed reply followed by a live reply.
I traced the fixes and their surrounding lifecycle contracts, checked the rebase delta, and integrated an independent reconnect-path source review. CI at this exact head passes 425 Vitest files / 5,130 tests; the updated keyboard/exact-navigation browser journeys passed in Chromium and WebKit. Diff whitespace checks pass. No local tests or app launch were repeated for this follow-up. The production-broker disconnect/recovery reproduction was not rerun; native WKWebView, reduced motion, momentum scrolling, and fresh human acceptance of the final styling remain deferred. The prior managed-agent metadata objection remains withdrawn.
…t-update-drafts * commit '0a4982797f38164d75e3e8f48e58fabb9dd59e66': (66 commits) 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) Fix flaky WebKit menu focus browser test (#409) Test Goose connections and fix Pi test false failures (#383) feat: open threads with verified newest-first windows (#154) Add agent conversation context selection (#382) test: keep behavioral coverage without cosmetic matrices (#410) Fix reading position and composer caret on channel return (#411) fix(channels): prevent clipped activity rows and remove separators (#377) ci: publish signed macOS updater artifacts in prereleases (#387) feat(messages): add jump to latest controls (#374) Align reply summaries with message content (#408) Add centered thinking pills to agent avatars (#351) Keep focus where the user moved it when a menu finishes closing (#355) Browse legacy identities without a destination and review text before cloning (#285) Show separate identity cards and prevent duplicate imports (#225) Polish message and thread spacing, grouping, and typography (#364) Remove the Away avatar badge stroke (#395) fix(profiles): hide activity on human profiles (#391) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
* origin/main: (25 commits) 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) Fix flaky WebKit menu focus browser test (#409) Test Goose connections and fix Pi test false failures (#383) feat: open threads with verified newest-first windows (#154) Add agent conversation context selection (#382) test: keep behavioral coverage without cosmetic matrices (#410) Fix reading position and composer caret on channel return (#411) fix(channels): prevent clipped activity rows and remove separators (#377) ci: publish signed macOS updater artifacts in prereleases (#387) feat(messages): add jump to latest controls (#374) Align reply summaries with message content (#408) Add centered thinking pills to agent avatars (#351) Keep focus where the user moved it when a menu finishes closing (#355) Browse legacy identities without a destination and review text before cloning (#285) Show separate identity cards and prevent duplicate imports (#225) Polish message and thread spacing, grouping, and typography (#364) Remove the Away avatar badge stroke (#395) fix(profiles): hide activity on human profiles (#391) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentsPage.test.tsx # src/bundled/agents/AgentsPage.tsx
Summary
Review feedback addressed
Verification
e88b0ac(maincfbb6124): full Vitest 419 files / 5,088 tests pass; 58 affected Chromium/WebKit browser tests pass; pre-push TypeScript, related unit tests and design guards pass.git diff --checkclean.Deferred
Native WKWebView, reduced motion and momentum scrolling were not exercised. Hosted CI for this head and fresh teammate review/approval remain pending. No merge authorization.