Repository navigation
Conversation
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One non-blocking focus-return issue.
c71731e to
1560808
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility). Reviewed head 1560808a5717e3e65b6a5df8690c5924e685169e, base 028e5a13d3b85eb8f042b80109e96514d622c783 (PR merge-base fe3dd1947181de66dc6e4fd8ca5d3b61573c0ca2).
The Channels return-target propagation addresses the previously reported path. One residual, non-blocking P3 in Activity’s PageTabs host is detailed inline as a continuation of the existing focus-return issue.
Source-only: inspected production changes, callers, lifecycle and regression assertions; did not run tests, builds, apps or CI. Browser/native focus ordering and rendered accessibility remain unverified. The PR description was inspected; its attached demo was not viewable (attachment redirect returned HTTP 403). Author-reported test/demo results are not independently verified by this review.
1560808 to
45ee5f4
Compare
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One remaining focus-return lifecycle issue.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Focused follow-up at 45ee5f4cfa658d60fd5d270131658ebdb5e45b2a against fb9cd7cf45c6d80368cc79a7c34535f8f3b2fd20. The previously reported Activity main-image → comment link → sole side-thread close path is repaired in source. The new PageTabs component and browser journeys cover that path; prior feature and Channels-path review coverage is retained.
Two nonblocking P3 cases remain in the new return-target ownership: verified reply roots can lose the association, and a main-pane close can leave it available to a later ordinary reopening. Details are inline. Neither is a blocker.
Source-only: inspected the repair, callers, regression source and public description. No tests, builds, app/browser runs, CI validation or rendered accessibility checks were performed; author-reported results are unverified here. The attached video could not be inspected because its redirect returned HTTP 403.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Focused repair review at 0a5d1889c4b651e41bc696a39d1b3163903bf75e against fb9cd7cf45c6d80368cc79a7c34535f8f3b2fd20. No new actionable findings. Both P3s from review 5798758869 are addressed in source: the return target follows verified-root reconciliation and merging, and it is retired when the thread leaves the open-tab set, including main-pane and bulk closes. The added PageTabs and hook cases cover those transitions and reopening; prior feature, Channels-path and browser-journey coverage is retained.
Source-only: inspected the repair, callers, regression source and public description. No tests, builds, app/browser runs, CI validation or rendered accessibility checks were performed. Author-reported validation remains unverified here. The attached historical video could not be inspected because its redirect returned HTTP 403.
0a5d188 to
6ed2d4f
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account.
Integration follow-up at 6ed2d4fb56f702ed319ad5dfa9ef1cc126961879 against base/merge-base cae3922574c5c1f0ce5d32231810229e94b1bb40: no new actionable findings.
The prior repairs remain intact: the media return target follows verified-root reconciliation and retires when its thread leaves the tab store, including main-pane and bulk closes. The rebased media attachment/timecode gates, explicit channel-root reveal and retained panel/drawer ownership do not introduce a source-visible conflict with comment-link opening or focus return.
Rocket reviewed host/tab/panel lifecycle; Mantis reviewed media identity and refusal/dismissal; Groot reviewed destination focus and regression coverage; Star Lord integrated. Prior feature and Channels-path coverage is retained.
Validation is source-only. Component and browser regression assertions were inspected, not run. Current-head browser/native focus, rendered accessibility and CI were not verified. The PR description and sampled frames from the historical fixture demo were inspected; that demo predates this head and does not establish current runtime behavior.
|
🤖 Drip design review: no must-fix design findings at People can follow media-comment links, mentions and avatars into the existing destinations. Closing the review after a successful internal open follows the navigation contract; refused links retain their status in the viewer. The surviving attachment supplies the return target, including Activity thread tabs, resolved reply roots and closed-tab cleanup. This fits the launch policy as a contained bug fix. Evidence: comment handoff, tab return and cleanup, and sampled frames from the attached historical demo. Unchecked: current-head runtime/native focus, WebKit, screen-reader output, dark mode, narrow layouts, enlarged text, long/many-agent content and rendered empty/loading/error states. The demo predates this head. Regression assertions were inspected; tests and CI were not run. |
6ed2d4f to
92fd1b4
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source-review follow-up via Wes’s account, with independent Rocket, Mantis and Groot coverage.
Reviewed head 92fd1b46676b445c82aecec2b6f50cf657404328 against base/merge-base 80cb3a25569f763a2b36ce473320fe628a17c85f, retaining the earlier feature review at 6ed2d4fb.
No new actionable findings in this integration follow-up. The four feature patches retain their earlier behavior. The new base’s media-comment reading anchor changes scrolling without replacing the opener; profile actions still use the wrapped host opener and close the review only on acceptance. Retained-pane visibility remains separate from the media return target. Both prior focus repairs remain intact: association follows verified thread roots and retires when the thread leaves the tab store.
Validation is source-only. Relevant regression assertions and the Activity browser journey were inspected, not executed. Tests, CI, current browser/native focus, rendered accessibility, and combined retained-drawer/media behavior remain unverified. The PR description and sampled cached frames of its historical fixture demo were inspected; that demo is for an older revision, not runtime evidence for this head.
92fd1b4 to
7b0c946
Compare
In the image and video viewers, comment rows ignored clicks on their
content. A Buzz message link showed "This Buzz link couldn't be opened
here.", @mentions were plain text, avatars were not buttons, and internal
links had no Open link menu. The same replies in the thread panel opened
all of these.
The viewer now passes its host's onOpenLink, with the row's { rootId }
origin, and canOpenLink to the comment rows. It also wraps the host's
thread-tab opener instead of hiding it. When the host reports that it
opened the link, the review closes, as following a link leaves a dialog
(docs/navigation-history.md). A refused link, or one the browser opens,
leaves the review open. "Open image in browser" is unchanged.
After a followed link, the viewer hands the modal boundary no explicit
return target, so focus stays where the destination put it, as it does
from the thread panel. Closing with x, Escape or the backdrop still
returns focus to the opener.
ConversationMediaReview, which renders the review for every host, now
also passes canOpenLink. In ConversationHost, the review's close only
clears a media overlay, so it no longer wipes the panel that a comment
link just opened.
This reverses the inert-comment behavior from #1113. Its reason, that no
host opener could reach the comments behind the review, no longer holds
once the review closes on success. The test that pinned it is replaced.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Following a media-review comment's profile, mention, link or Open link closes the review and opens a destination. Closing that destination then sent focus to the page fallback, Channel actions, even though the attachment that opened the review was still on the page. The host had recorded the comment control as its return target, and that control unmounts with the review. The review now names its opener as the return target through the host's existing return-focus contract, as the Members dialog does. Its comment rows add returnFocus to the link origin they hand onOpenLink. Its thread-tab wrapper passes the opener as a new optional fourth argument to the LinkThreadTabContext opener. MessageLinkOrigin gains an optional returnFocus, and its rootId becomes optional so an origin without a thread, such as an agent's View activity, can carry one. Channels already preferred returnFocus for panel openings. It now also prefers it for routed message links and linked thread tabs. The shared tab owner used by Bestie and conversation tabs prefers it for panels and forwards it to its host's link opener. ConversationHost already returns focus to the first overlay's opener, so it is unchanged. Initial focus is unchanged: the destination keeps focus when the review closes. Closing the destination returns focus to the media opener. New ChannelsPage tests open a real review and follow a comment's profile, its Open link and its routed message link. Each one then closes the destination and expects focus on the image attachment. Viewer and tab-owner tests pin the forwarded return target. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…thread In Activity, a media review opened from a thread in the main pane could follow a comment's message link with Open link into a side thread. Closing that only side tab sent focus to the pane toggle or the page fallback, even though the image that opened the review was still on the page. The review passes its opener to the thread-tab opener as a fourth argument. Activity's PageTabs hands the shared owner's openLinkedThread to LinkThreadTabContext as is, and that opener took only three arguments, so the target was dropped. The shared tab owner, useConversationTabs, now keeps the return target a linked thread's link names, for that thread. When its tab is the last one in the side pane, closing it focuses that target instead of the pane toggle. This matches how the owner already returns link-opened panels to their returnFocus. A target that is gone, hidden or inert leaves the previous pane focus. Closing the thread retires its target, so the same thread opened later from elsewhere does not reuse it. Channels wraps the opener and returns focus itself, so it is unchanged. Bestie also passes the owner's opener directly and now hands it the target too, but its own close handler still decides focus there. A new ChannelsPage.media test mounts PageTabs as Activity does, moves the image thread to the main pane, follows the comment's Open link and closes the side thread. A tab-owner test covers the hidden target and the retirement. A browser journey in inbox-tabs.spec.mjs checks the same flow with native focus in Chromium and WebKit. All three fail without the fix. The existing Channels mount() helper in ChannelsPage.media.test.tsx also gains the passThrough stub that PageNavigation requires; without it, tsc --noEmit failed on that file at the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A media review's comment can open a thread with Open link and name the image as the place to return focus. The shared tab owner, useConversationTabs, kept that target for the thread's root until the thread closed from the side pane. Two review findings showed where it went wrong. The target outlived its tab when the tab left another way. Closing the thread in the main pane, or with Close all or Close other tabs, kept it. If the same thread was then opened again from an Activity row and closed as the only side tab, focus went to the old image instead of the pane's usual return. Every path that removes the tab now retires the target. The owner already ends a closed tab's mounted state each time it renders, and the target now ends there too, once no open thread tab holds its root. The target was also lost when a link pointed at a reply. Without a thread hint, the link names the reply as the root. When the reader reports the real root, the tab adopts it, and the stored root no longer matched. Closing the only side tab then skipped the image. The target now follows the verified root, including when the tab merges into the thread's already-open tab. Each pane keeps its focus policy: only closing the last side tab returns focus to the target. Channels never hands the owner a target, so it is unchanged. Tab IDs are reused when a closed thread opens again, so keying the target by tab ID would not have retired it. The owner keys it by channel and verified root instead. New tests: - useConversationTabs.test.tsx: the target survives a move to the main pane and back. A main-pane close, Close all and Close other tabs each retire it. - PageTabs.test.tsx: a thread linked at a reply returns focus to the target after its root resolves, in its own tab and after merging into the thread's open tab. All five fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
7b0c946 to
f0a8901
Compare
In the image and video viewers, clicks on comment content did nothing. A Buzz message link showed "This Buzz link couldn’t be opened here.", an @mention was plain text, a commenter's avatar wasn't a button, and internal links had no Open link menu. In the thread panel, the same replies opened all of these.
The viewer now passes its host's
onOpenLinkandcanOpenLinkto the comment rows, along with the row's{ rootId }origin. It also wraps the host's thread-tab opener instead of hiding it. When the host opens the destination, the viewer closes.Before / after
Left:
mainat b621738. Right: this branch. Both run the same scripted flow (Chromium, local fixture relay) at the same times. Step 2: the Buzz link. Step 3: the @mention. Step 4: the avatar. Step 5: Shift+F10 → Open link.staged-image-3edfc98d-5625-4cc4-aa54-1963fc29444e.mp4
Behaviour
ConversationMediaReviewnow passescanOpenLinkfor every host.ConversationHost, the review's close only clears a media overlay. It no longer wipes the panel that a comment link just opened.Why the viewer closes
docs/navigation-history.mdsays that following a link which exits a dialog ends that dialog. For example, Members → profile → Back returns to the page without Members. While the review is open, the page behind it is inert and hidden from assistive tech, so a destination opened behind the review could not be seen or reached. The review is not a history entry, so Back does not reopen it.This reverses the decision from #1113 to keep comment links inert. Its reason was that no host opener could reach the comments behind the review, and that no longer holds once the review closes on success. The test that pinned the old behaviour, "keeps comment message links inert beneath a host tab opener", is replaced by "opens comment message links in a host thread tab, then closes".
Testing
ConversationHostguard or theConversation.tsxpass-through, the 2EmbeddedThreadcases fail.MediaReviewViewer*,src/features/conversation,ImageReviewStage,useModalBoundary,ConversationTab,DraftsView,PinnedBestie).tsc --noEmitandbiome check --error-on-warningsare clean.Not covered
ConversationHostsurfaces (Inbox DM detail, drafts, pinned Bestie) with a real panel. The unit tests cover these with a mocked viewer.🤖 Generated with Claude Code