fix(web): close hover tooltips when the chat timeline scrolls - #7860
fix(web): close hover tooltips when the chat timeline scrolls#7860Exotic209093 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| threadId: event.payload.threadId, | ||
| requestId, | ||
| turnId, | ||
| createdAt: event.payload.createdAt, |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderCommandReactor.ts:1272
The synthetic user-input.resolved activity can sort before its corresponding user-input.requested, leaving the request pending after a successful interrupt. This happens because it reuses event.payload.createdAt, which may equal or precede the request timestamp; use a server-generated ordering/timestamp that is guaranteed to follow the request.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 1272:
The synthetic `user-input.resolved` activity can sort before its corresponding `user-input.requested`, leaving the request pending after a successful interrupt. This happens because it reuses `event.payload.createdAt`, which may equal or precede the request timestamp; use a server-generated ordering/timestamp that is guaranteed to follow the request.
| yield* providerService.interruptTurn({ threadId: event.payload.threadId }); | ||
| // Some providers discard their callbacks without emitting a matching | ||
| // resolution event. Close those requests after the interrupt succeeds. | ||
| yield* Effect.forEach(pendingUserInputRequests(thread.activities), ({ requestId, turnId }) => |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderCommandReactor.ts:1267
interruptTurn can emit user-input.resolved itself, but this loop uses the pre-interrupt thread.activities snapshot and appends another resolution for the same requestId, causing duplicate entries such as both “submitted” and “cancelled”. Re-read the thread’s pending activities after the interrupt before appending synthetic cancellations.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 1267:
`interruptTurn` can emit `user-input.resolved` itself, but this loop uses the pre-interrupt `thread.activities` snapshot and appends another resolution for the same `requestId`, causing duplicate entries such as both “submitted” and “cancelled”. Re-read the thread’s pending activities after the interrupt before appending synthetic cancellations.
There was a problem hiding this comment.
Reviewed the web UI changes (apps/web/src/components/ui/tooltip.tsx, apps/web/src/components/chat/MessagesTimeline.tsx). The scroll-dismiss behavior added to the shared Tooltip primitive has two contract/behavior concerns; details inline.
Posted via Macroscope — UI Consistency
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 947726b6e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| threadId: event.payload.threadId, | ||
| requestId, | ||
| turnId, | ||
| createdAt: event.payload.createdAt, |
There was a problem hiding this comment.
When a remote client's clock is behind the server, event.payload.createdAt can precede the provider-generated user-input.requested timestamp because command metadata uses the client clock (packages/client-runtime/src/operations/commands.ts:72-81). Activity state is reconstructed chronologically (apps/server/src/orchestration/projector.ts:171-185), so this cancellation is processed before the request and the question remains pending after Stop. Generate a server-side timestamp or otherwise guarantee lifecycle ordering for the resolution.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
| yield* providerService.interruptTurn({ threadId: event.payload.threadId }); | ||
| // Some providers discard their callbacks without emitting a matching | ||
| // resolution event. Close those requests after the interrupt succeeds. | ||
| yield* Effect.forEach(pendingUserInputRequests(thread.activities), ({ requestId, turnId }) => |
There was a problem hiding this comment.
When stopping a Cursor or Grok turn with an open question, their adapters already settle pending input during interruption (CursorAdapter.ts:1067-1072 and GrokAdapter.ts:1328-1332), which resumes the request handler and emits user-input.resolved. This loop still uses the thread snapshot loaded before interruptTurn, so it unconditionally persists another, contradictory "User input cancelled" resolution; both durable activities then render in the timeline. Re-read pending state after the interrupt or synthesize cancellation only for adapters that do not resolve it themselves.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
| expect(harness.interruptTurn.mock.calls[0]?.[0]).toEqual({ | ||
| threadId: "thread-1", | ||
| }); | ||
| await waitFor(async () => { |
There was a problem hiding this comment.
This new assertion polls the asynchronously projected read model with the test's ten-second timeout even though the harness exposes drain(). That makes the test timing-dependent under slow CI and violates the event-sourced test requirement to wait on receipts or worker drains; dispatch the interrupt and await the reactor drain before asserting the activity.
AGENTS.md reference: AGENTS.md:L109-L109
Useful? React with 👍 / 👎.
| function pendingUserInputRequests( | ||
| activities: OrchestrationThread["activities"], | ||
| ): ReadonlyArray<{ readonly requestId: string; readonly turnId: TurnId | null }> { |
There was a problem hiding this comment.
The proposed change is titled as a web tooltip fix, but this helper and its companion reactor changes independently alter durable server behavior whenever a turn is stopped. Keeping these concerns together makes the server behavior easy to miss during review and impossible to revert independently from the tooltip change; move it and its test into a separate change.
AGENTS.md reference: AGENTS.md:L119-L119
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 947726b. Configure here.
| turnId, | ||
| createdAt: event.payload.createdAt, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Duplicate user-input cancel on interrupt
Medium Severity
Pending user-input cancellation after interrupt always uses the pre-interrupt thread.activities snapshot, so every open request is closed with a cancelled user-input.resolved even when the provider already emits its own resolution during interruptTurn. For Claude, abort already publishes user-input.resolved, so interrupt now leaves both a cancelled activity and a submitted one for the same requestId.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 947726b. Configure here.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This PR adds scroll-dismiss behavior for tooltips in the chat timeline - a self-contained UI fix. The implementation correctly uses React context with a child listener component, and the Macroscope feedback about context consumption was addressed. Only two frontend files are changed with no backend impact. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
947726b to
459564f
Compare
github-actions
Bot
added
size:M
459564f to
9bb16e9
Compare
github-actions
Bot
added
size:L
|
|
||
| /** Dismisses every hover-opened tooltip under the nearest scope. Null outside one. */ | ||
| export function useTooltipScrollDismiss(): (() => void) | null { | ||
| const dismiss = use(TooltipScrollDismissContext); |
There was a problem hiding this comment.
🟡 Medium ui/tooltip.tsx:60
dismissTooltips is always null, so MessagesTimeline never installs its wheel, touch, pointer, or keyboard dismissal listeners and scrolling leaves hover tooltips open. useTooltipScrollDismiss() runs before the returned TooltipScrollDismissScope, which is a descendant and cannot provide context to this hook call. Move the hook/listener logic into a child rendered beneath the scope, or place the scope above MessagesTimeline.
Also found in 1 other location(s)
apps/web/src/components/chat/MessagesTimeline.tsx:436
useTooltipScrollDismiss()is called byMessagesTimelinebefore (and therefore outside) theTooltipScrollDismissScopethat this same component returns. React context is read only from providers above the calling component, not providers in its returned subtree, sodismissTooltipsis alwaysnullhere and the effect installs no gesture listeners. Timeline scrolling therefore still leaves hover tooltips open.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/tooltip.tsx around line 60:
`dismissTooltips` is always `null`, so `MessagesTimeline` never installs its wheel, touch, pointer, or keyboard dismissal listeners and scrolling leaves hover tooltips open. `useTooltipScrollDismiss()` runs before the returned `TooltipScrollDismissScope`, which is a descendant and cannot provide context to this hook call. Move the hook/listener logic into a child rendered beneath the scope, or place the scope above `MessagesTimeline`.
Also found in 1 other location(s):
- apps/web/src/components/chat/MessagesTimeline.tsx:436 -- `useTooltipScrollDismiss()` is called by `MessagesTimeline` before (and therefore outside) the `TooltipScrollDismissScope` that this same component returns. React context is read only from providers above the calling component, not providers in its returned subtree, so `dismissTooltips` is always `null` here and the effect installs no gesture listeners. Timeline scrolling therefore still leaves hover tooltips open.
There was a problem hiding this comment.
Confirmed — the listener was consuming context from outside the scope it rendered, so dismissal could never activate. The hook now lives in a small TooltipScrollDismissListener child rendered inside TooltipScrollDismissScope (MessagesTimeline), wired to the viewport element it already tracks. Typecheck clean, MessagesTimeline tests 26/26.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
One blocking finding: the scroll-dismiss wiring is dead at runtime because the hook is called outside the scope it renders, so the timeline never dismisses hover tooltips.
Posted via Macroscope — UI Consistency
9bb16e9 to
64b0366
Compare
There was a problem hiding this comment.
Reviewed the tooltip primitive change and its timeline call site. The scope/listener split now resolves correctly (the listener is a descendant of the provider, so useTooltipScrollDismiss() is non-null), and gating registration on details.reason === "trigger-hover" does exempt focus/keyboard opens ("trigger-focus") as documented. Two findings on the shared-primitive contract below.
Posted via Macroscope — UI Consistency
| const handleOpenChange = (open: boolean, details: TooltipPrimitive.Root.ChangeEventDetails) => { | ||
| onOpenChange?.(open, details); | ||
| registeredCloseRef.current?.(); | ||
| registeredCloseRef.current = null; | ||
| if (!scrollDismiss || !open || details.reason !== "trigger-hover") { | ||
| return; | ||
| } | ||
| const close = () => actionsRef.current?.close(); | ||
| registeredCloseRef.current = scrollDismiss.register(close); |
There was a problem hiding this comment.
This changes behavior of a primitive every tooltip in the app renders through — prop forwarding (actionsRef write-through, onOpenChange interception) and open/close state transitions — but no test accompanies it. The repo already has focused primitive tests (apps/web/src/components/ui/menu.test.tsx, button.test.tsx) and timeline tests (apps/web/src/components/chat/MessagesTimeline.test.tsx); a small test would pin the contract: a hover-opened tooltip inside TooltipScrollDismissScope closes on wheel over the scope node, a plain programmatic scroll does not close it, a focus-opened tooltip is unaffected, and a consumer-supplied actionsRef still receives the actions object.
Posted via Macroscope — UI Consistency
| return dismiss?.dismissAll ?? null; | ||
| } | ||
|
|
||
| function Tooltip(props: TooltipPrimitive.Root.Props) { |
There was a problem hiding this comment.
Wrapping TooltipPrimitive.Root in a non-generic component narrows the primitive's public prop surface: before this change Tooltip was TooltipPrimitive.Root, whose props are Root.Props<Payload>, so handle was TooltipHandle<Payload> and a render-function child received a typed payload. Now Payload collapses to unknown, which makes the TooltipCreateHandle export (createTooltipHandle<Payload>()) unusable through this wrapper — <Tooltip handle={TooltipCreateHandle<MyPayload>()}> no longer type-checks. Keeping the type parameter preserves the contract at no runtime cost:
| function Tooltip(props: TooltipPrimitive.Root.Props) { | |
| function Tooltip<Payload>(props: TooltipPrimitive.Root.Props<Payload>) { |
(Root.Actions isn't payload-generic, so mergedActionsRef and the rest of the body are unaffected.)
Posted via Macroscope — UI Consistency
1 participant
Hovering a Markdown link in the chat opens its tooltip, and scrolling the timeline without moving the pointer leaves the tooltip open after its trigger has scrolled away. Since tooltips portal above the chat surface, it then paints over the composer and its controls.
Tooltips now opt into scroll dismissal through a
TooltipScrollDismissScopethat wraps the messages timeline: when a tooltip opens via hover inside that scope, it attaches a capture-phasescrolllistener ondocument(so inner scrollers are covered) and closes itself on the first scroll event, detaching cleanly right after and on unmount. Tooltips opened by keyboard focus are untouched, so keyboard users keep them until focus moves, and tooltips outside the timeline behave exactly as before.Fixes #7767
ox-alpha via opencode
Note
Close hover tooltips on real scroll gestures in
MessagesTimelineTooltipScrollDismissScopeanduseTooltipScrollDismissto tooltip.tsx. A new context tracks hover-opened tooltips and exposes adismissAllcallable.Tooltipcomponent so hover-opened tooltips register their close callback with the nearest scope and unregister on close or unmount.actionsRefis proxied to preserve consumer-supplied refs.TooltipScrollDismissListenerand wraps timeline content in MessagesTimeline.tsx. The listener callsdismissAllonwheel,touchmove,pointerdown, and keydown for scroll keys (PageUp,PageDown,Home,End,ArrowUp,ArrowDown).Tooltipis no longer a direct alias ofTooltipPrimitive.Root; it is a wrapper that interceptsonOpenChangeand managesactionsRefinternally. Consumers outside aTooltipScrollDismissScopesee unchanged behavior.Macroscope summarized 64b0366.