fix(SelectionContext): keep selection tooltip alive after plugin view re-creation#1171
Open
resure wants to merge 2 commits into
Open
fix(SelectionContext): keep selection tooltip alive after plugin view re-creation#1171resure wants to merge 2 commits into
resure wants to merge 2 commits into
Conversation
|
|
resure
added a commit
to resure/gravity-notes
that referenced
this pull request
Jul 2, 2026
The virtualized note list now renders only an IconPickerButton glyph per row and shares ONE controlled IconPickerPopup (the split lives in IconPicker.tsx) — a per-row picker was a large render cost and an open one died when its row left the virtual window. Plus fixes from a max-effort review of the change: - editorCaret: the no-rect first-line fallback treats block boundaries as line breaks (an empty second bullet/blockquote line no longer yanks ArrowUp to the title), skips ProseMirror-trailingBreak placeholders (a caret at (p,1) on a blank line still hands off), gates on the collapsed range start (backward selections), and scans with comparePoint instead of deep-cloning via cloneContents - NoteList: Enter on a focused row button activates the button instead of opening the note; the ⋯ menu gains the picker's anchor keepalive + close-when-note-leaves; the shared popup is memoized with stable handlers (live-ref) and drops picks whose note id went stale - IconPicker: popup resets query/tab/scroll/highlight on every close so no state leaks across rows; open derived from the anchor - Workspace: F2 no-ops inside any floating layer (.g-popup / [role=dialog]) instead of a per-widget class blocklist - docs: EDITOR_BUG_REPORT.md superseded by the filed upstream PR (gravity-ui/markdown-editor#1171, linked in selectionContextFix.ts); HANDOFF.md follow-ups moved to README (fonts-CSP known limitation, icon-picker backlog); note-icons feature bullet added Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resure
force-pushed
the
fix/selection-context-tooltip-rearm
branch
from
255ed54 to
00d197b
Compare
…w re-creations and native context menus ProseMirror destroys and re-creates plugin views whenever state.plugins changes identity (any host-side EditorState.create, even one that passes the current plugins through). The spec's destroyed flag stayed true after that, so the mouse release was ignored, the press gate stuck, and the tooltip never showed again. Re-arm the flag in view() and re-evaluate on release without the mousedown snapshot, so a click that leaves the doc/selection unchanged re-shows the tooltip instead of stranding it hidden. Rework the press gate around a single armPressGate helper that owns its document listeners via AbortController: - treat ctrl+click as a context-menu press: macOS reports it with button 0 while the native menu still swallows the mouseup, which left the gate stuck - gate updates during a non-primary press so the browser's right-click word-selection doesn't pop the tooltip up under the open native menu; the gate releases on the next input that reaches the page - keep the gate armed across a mid-press plugin view re-creation instead of resetting it, so a host-side state swap during a drag doesn't un-gate updates - disarm the pending mouseup listener on dragstart instead of leaving it to fire on the next unrelated mouseup - ignore chorded secondary-button releases and releases whose editor was destroyed while the button was held
resure
force-pushed
the
fix/selection-context-tooltip-rearm
branch
from
00d197b to
089e9a7
Compare
Reviewer's GuideRefactors SelectionContext’s tooltip press-handling to use a reusable gate mechanism that survives plugin view re-creations, correctly re-evaluates state on mouseup, and robustly handles context-menu presses and drags, with new integration-style tests verifying behavior across editor state swaps and diverse input patterns. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The press-gating logic (armPressGate + handleDOMEvents.mousedown/dragstart) has grown fairly complex; consider extracting it into a small helper module or at least a separate class to keep the plugin spec focused on selection behavior and make the gate’s lifecycle easier to reason about.
- Tests currently mock platform behavior via navigator.platform while the implementation uses isMac(); stubbing isMac() directly in tests would better match the production path and avoid relying on navigator.platform quirks across environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The press-gating logic (armPressGate + handleDOMEvents.mousedown/dragstart) has grown fairly complex; consider extracting it into a small helper module or at least a separate class to keep the plugin spec focused on selection behavior and make the gate’s lifecycle easier to reason about.
- Tests currently mock platform behavior via navigator.platform while the implementation uses isMac(); stubbing isMac() directly in tests would better match the production path and avoid relying on navigator.platform quirks across environments.
## Individual Comments
### Comment 1
<location path="packages/editor/src/extensions/behavior/SelectionContext/index.ts" line_range="78" />
<code_context>
private hideTimeoutRef: ReturnType<typeof setTimeout> | null = null;
- private _isMousePressed = false;
+ private _releasePressGate: (() => void) | null = null;
constructor(
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the dynamic `armPressGate`/`_releasePressGate` gating logic with an explicit press-state machine and fixed document listeners to manage tooltip updates during input.
The new `armPressGate` + `_releasePressGate` flow does materially increase complexity; you can get the same behavior with a simpler, explicit state machine and fixed listeners.
Instead of dynamically arming document listeners with a generic `{[eventType]: predicate}` map per press, consider:
1. Make press state explicit.
2. Install a small, fixed set of document listeners once.
3. Drive behavior from `pressState` inside those listeners and `update`.
This keeps all the “gate until next input” semantics, but removes `AbortController`, the indirection via predicates, and the need to reason about which listeners are currently armed.
### 1. Make press state explicit
```ts
type PressState =
| 'idle'
| 'primaryPress'
| 'contextMenuPress'
| 'dragging';
class SelectionTooltip implements PluginSpec<PluginState> {
private pressState: PressState = 'idle';
private teardownPressListeners: (() => void) | null = null;
// ...
}
```
### 2. Register fixed document listeners once
Install them when the plugin view is created, and tear them down on real editor destroy. This replaces `armPressGate` and `_releasePressGate`:
```ts
view(view: EditorView) {
this.update(view);
const onDocMouseUp = (event: MouseEvent) => {
if (this.pressState === 'primaryPress' && event.button === 0) {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
} else if (this.pressState === 'contextMenuPress' && event.button === 0) {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
const onDocMouseDown = () => {
if (this.pressState === 'contextMenuPress') {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
const onDocKeyDown = () => {
if (this.pressState !== 'idle') {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
document.addEventListener('mouseup', onDocMouseUp, {capture: true});
document.addEventListener('mousedown', onDocMouseDown, {capture: true});
document.addEventListener('keydown', onDocKeyDown, {capture: true});
this.teardownPressListeners = () => {
document.removeEventListener('mouseup', onDocMouseUp, {capture: true} as any);
document.removeEventListener('mousedown', onDocMouseDown, {capture: true} as any);
document.removeEventListener('keydown', onDocKeyDown, {capture: true} as any);
};
return {
update: this.update.bind(this),
destroy: () => {
this.cancelTooltipHiding();
this.tooltip.destroy();
queueMicrotask(() => {
if (view.isDestroyed) {
this.pressState = 'idle';
this.teardownPressListeners?.();
this.teardownPressListeners = null;
}
});
},
};
}
```
This preserves the “survive plugin view re-creations, but not editor teardown” behavior without storing closures per press or an `AbortController`.
### 3. Drive press state via `handleDOMEvents`
`mousedown` sets the state explicitly instead of arming a gate:
```ts
handleDOMEvents: {
mousedown: (view, event) => {
this.cancelTooltipHiding();
this.tooltip.hide(view);
if (isContextMenuPress(event)) {
// native menu may swallow mouseup; treat as contextMenuPress
this.pressState = 'contextMenuPress';
return;
}
// chorded secondary-button releases can be handled inside the fixed mouseup handler
this.pressState = 'primaryPress';
},
dragstart: () => {
// drag ends with dragend, not mouseup; we just mark dragging
this.pressState = 'dragging';
},
},
```
You can handle `dragend` either with another fixed document listener or a `view.dom` listener:
```ts
const onDocDragEnd = () => {
if (this.pressState === 'dragging') {
this.pressState = 'idle';
}
};
document.addEventListener('dragend', onDocDragEnd, {capture: true});
```
### 4. Simplify `update`
With explicit `pressState`, you no longer need `_releasePressGate` or an `EditorState` param:
```ts
private update(view: EditorView) {
this.editorView = view;
// A press interaction is in flight (button held, or a native context menu possibly open)
if (this.pressState !== 'idle') return;
this.cancelTooltipHiding();
const hideFromTr = pluginKey.getState(view.state)?.disabled;
if (hideFromTr || !view.dom.parentNode) {
this.tooltip.hide(view);
return;
}
// ...rest unchanged
}
```
This keeps behavior (don’t update while a press is in flight) but removes the hidden gating via `_releasePressGate` and the need for the generic `armPressGate` API.
---
Overall effect:
- Same robustness around context menu presses, macOS ctrl+click, and drag.
- No dynamic listener maps or `AbortController` per press.
- No need to mentally simulate predicate logic across multiple “armed” gates.
- Tooltip behavior is now a straightforward function of `pressState` + `EditorState`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| private hideTimeoutRef: ReturnType<typeof setTimeout> | null = null; | ||
|
|
||
| private _isMousePressed = false; | ||
| private _releasePressGate: (() => void) | null = null; |
There was a problem hiding this comment.
issue (complexity): Consider replacing the dynamic armPressGate/_releasePressGate gating logic with an explicit press-state machine and fixed document listeners to manage tooltip updates during input.
The new armPressGate + _releasePressGate flow does materially increase complexity; you can get the same behavior with a simpler, explicit state machine and fixed listeners.
Instead of dynamically arming document listeners with a generic {[eventType]: predicate} map per press, consider:
- Make press state explicit.
- Install a small, fixed set of document listeners once.
- Drive behavior from
pressStateinside those listeners andupdate.
This keeps all the “gate until next input” semantics, but removes AbortController, the indirection via predicates, and the need to reason about which listeners are currently armed.
1. Make press state explicit
type PressState =
| 'idle'
| 'primaryPress'
| 'contextMenuPress'
| 'dragging';
class SelectionTooltip implements PluginSpec<PluginState> {
private pressState: PressState = 'idle';
private teardownPressListeners: (() => void) | null = null;
// ...
}2. Register fixed document listeners once
Install them when the plugin view is created, and tear them down on real editor destroy. This replaces armPressGate and _releasePressGate:
view(view: EditorView) {
this.update(view);
const onDocMouseUp = (event: MouseEvent) => {
if (this.pressState === 'primaryPress' && event.button === 0) {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
} else if (this.pressState === 'contextMenuPress' && event.button === 0) {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
const onDocMouseDown = () => {
if (this.pressState === 'contextMenuPress') {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
const onDocKeyDown = () => {
if (this.pressState !== 'idle') {
this.pressState = 'idle';
if (!view.isDestroyed) this.update(view);
}
};
document.addEventListener('mouseup', onDocMouseUp, {capture: true});
document.addEventListener('mousedown', onDocMouseDown, {capture: true});
document.addEventListener('keydown', onDocKeyDown, {capture: true});
this.teardownPressListeners = () => {
document.removeEventListener('mouseup', onDocMouseUp, {capture: true} as any);
document.removeEventListener('mousedown', onDocMouseDown, {capture: true} as any);
document.removeEventListener('keydown', onDocKeyDown, {capture: true} as any);
};
return {
update: this.update.bind(this),
destroy: () => {
this.cancelTooltipHiding();
this.tooltip.destroy();
queueMicrotask(() => {
if (view.isDestroyed) {
this.pressState = 'idle';
this.teardownPressListeners?.();
this.teardownPressListeners = null;
}
});
},
};
}This preserves the “survive plugin view re-creations, but not editor teardown” behavior without storing closures per press or an AbortController.
3. Drive press state via handleDOMEvents
mousedown sets the state explicitly instead of arming a gate:
handleDOMEvents: {
mousedown: (view, event) => {
this.cancelTooltipHiding();
this.tooltip.hide(view);
if (isContextMenuPress(event)) {
// native menu may swallow mouseup; treat as contextMenuPress
this.pressState = 'contextMenuPress';
return;
}
// chorded secondary-button releases can be handled inside the fixed mouseup handler
this.pressState = 'primaryPress';
},
dragstart: () => {
// drag ends with dragend, not mouseup; we just mark dragging
this.pressState = 'dragging';
},
},You can handle dragend either with another fixed document listener or a view.dom listener:
const onDocDragEnd = () => {
if (this.pressState === 'dragging') {
this.pressState = 'idle';
}
};
document.addEventListener('dragend', onDocDragEnd, {capture: true});4. Simplify update
With explicit pressState, you no longer need _releasePressGate or an EditorState param:
private update(view: EditorView) {
this.editorView = view;
// A press interaction is in flight (button held, or a native context menu possibly open)
if (this.pressState !== 'idle') return;
this.cancelTooltipHiding();
const hideFromTr = pluginKey.getState(view.state)?.disabled;
if (hideFromTr || !view.dom.parentNode) {
this.tooltip.hide(view);
return;
}
// ...rest unchanged
}This keeps behavior (don’t update while a press is in flight) but removes the hidden gating via _releasePressGate and the need for the generic armPressGate API.
Overall effect:
- Same robustness around context menu presses, macOS ctrl+click, and drag.
- No dynamic listener maps or
AbortControllerper press. - No need to mentally simulate predicate logic across multiple “armed” gates.
- Tooltip behavior is now a straightforward function of
pressState+EditorState.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Text below and fix itself is mostly ai-generated, but I've also checked suggested fix manually on my project that uses markdown-editor
The floating selection toolbar (WYSIWYG mode, non-empty
selectionContextconfig) goes permanently dead after any host-sideEditorStateswap.The bug
SelectionTooltipkeeps two mutable flags on the extension instance —destroyedand_isMousePressed— and nothing re-arms them when its plugin view is re-created. ProseMirror re-creates every plugin view wheneverstate.pluginschanges identity, andEditorState.createalways builds a fresh plugins array — so any host-side state swap triggers this (e.g. an undo-history reset), even one that passesview.state.pluginsthrough unchanged:destroy()setsdestroyed = true. The view is re-created immediately, butview()never resets the flag._isMousePressed = true, and arms a one-shot documentmouseuplistener.if (this.destroyed) return— so_isMousePressedis never reset.update()exits atif (this._isMousePressed) return. The toolbar never appears again for the lifetime of the editor — no errors, nothing in the console.Minimal reproduction:
Two related flaws in the same handler:
update()skips when doc + selection are unchanged since then. But the mousedown just hid the tooltip — so a press-release cycle that keeps the selection (e.g. re-selecting the same word) strands the toolbar hidden over a live selection.mouseupat all — a right-click on macOS (the native context menu opens during mousedown and swallows the release), or a press that turns into a native drag (ends withdragend) — wedging_isMousePresseduntil the next completed left click.The fix
view(): re-armdestroyedand_isMousePressedwhen the plugin view is (re-)created — plugin views are re-created far more often than plugins are.dragstart.The plugin-view
updatepath keeps itsprevStateshort-circuit — that one is correct (the hide-meta check runs before it).Tests
The new
index.test.tsdrives the plugin through the publicSelectionContextextension with a realEditorViewin jsdom, including a realview.updateState(EditorState.create(...))swap for the main scenario — so it exercises ProseMirror's actual plugin-view re-creation. On currentmainthe baseline test passes and the four regression tests fail (one per flaw); with the fix all five pass. The package unit suite stays green.Found in gravity-notes, which currently ships a vendored copy of this plugin with these fixes; also verified end-to-end there in a packaged app with the plugin's gate decisions instrumented (before:
mousedown → mouseup → (nothing), toolbar dead; after:mousedown → mouseup → SHOW).Summary by Sourcery
Fix SelectionContext tooltip behavior so the floating selection toolbar remains functional after editor state swaps and various mouse interactions.
Bug Fixes:
Tests:
Summary by Sourcery
Ensure the floating selection tooltip remains functional and correctly gated around mouse interactions and editor state swaps.
Bug Fixes:
Tests: