You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
SdTooltip.vue's updatePosition() clamped the tooltip horizontally against the viewport but always placed it above the trigger (triggerRect.top - contentHeight - 10), with no vertical bounds check.
Fix: when the tooltip would not fit above the trigger (within the existing 8px viewport gutter), place it below (triggerRect.bottom + offset) instead. Placement is exposed as data-placement="top" | "bottom" on .sd-tooltip-content, which the component's own CSS uses to flip the arrow anchor and the transition's transform-origin so the tooltip still points at and grows from the trigger.
Note: this introduces data-placement as a small styling contract on the tooltip content element (same pattern as
floating-ui/Radix data-side). Pre-existing and untouched: the arrow is centered on the tooltip, so it can miss the trigger when the tooltip is horizontally clamped at the far left/right viewport edges.
Before / After
Toolbar flush with the top of the viewport, hovering the first toolbar button:
Before
After
Test plan
New unit tests in SdTooltip.test.js (positioning block): tooltip stays fully inside the viewport and clear of the trigger
when the trigger is flush with the top; still renders fully above when there is room; data-placement marks the side so the arrow points at the trigger. Verified the flush-top test fails without the fix (tooltip at top: -44px) and passes with it.
Existing SdTooltip auto-hide test unchanged and passing.
Full @superdoc/super-editor test suite: 1218 files / 17210 tests passed.
Manual check on the dev server with the toolbar at the top of the page: tooltip flips below the toolbar and the arrow points up at the button; with room above, behavior is unchanged.
The new positioning tests open SdTooltip instances but never close or unmount them, so the
component’s global resize/scroll/keydown listeners remain registered after the test finishes. This
can create cross-test contamination/flakiness and accumulate listeners across the suite.
ⓘ Recommendations generated based on similar findings in past PRs
Evidence
mountAndOpen() mounts and opens the tooltip but returns only the DOM node, leaving no unmount
path; meanwhile SdTooltip adds global listeners when isOpen is true and only removes them on
close/unmount. Since the positioning tests don’t close or unmount, the listeners persist beyond the
test cleanup (which only clears the DOM and restores mocks).
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`mountAndOpen()` mounts `SdTooltip` and opens it, but the tests never call `wrapper.unmount()` (nor do they close the tooltip). `SdTooltip` registers global `window`/`document` listeners while open, so these listeners can survive past the test and interfere with later tests.
### Issue Context
Clearing `document.body.innerHTML` does **not** unmount Vue components, so `onBeforeUnmount` cleanup never runs.
### Fix Focus Areas
- packages/super-editor/src/editors/v1/components/toolbar/SdTooltip.test.js[6-11]
- packages/super-editor/src/editors/v1/components/toolbar/SdTooltip.test.js[74-93]
### Suggested fix
- Track mounted wrappers and unmount them in `afterEach` (e.g., push each `wrapper` returned by `mount()` into an array and `unmount()` them during cleanup).
- Alternatively, change `mountAndOpen()` to return `{ wrapper, content }` and have each test call `wrapper.unmount()` in a `finally` block.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The new positioning tests open SdTooltip instances but never close or unmount them, so the
component’s global resize/scroll/keydown listeners remain registered after the test finishes. This
can create cross-test contamination/flakiness and accumulate listeners across the suite.
ⓘ Recommendations generated based on similar findings in past PRs
Evidence
mountAndOpen() mounts and opens the tooltip but returns only the DOM node, leaving no unmount
path; meanwhile SdTooltip adds global listeners when isOpen is true and only removes them on
close/unmount. Since the positioning tests don’t close or unmount, the listeners persist beyond the
test cleanup (which only clears the DOM and restores mocks).
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`mountAndOpen()` mounts `SdTooltip` and opens it, but the tests never call `wrapper.unmount()` (nor do they close the tooltip). `SdTooltip` registers global `window`/`document` listeners while open, so these listeners can survive past the test and interfere with later tests.
### Issue Context
Clearing `document.body.innerHTML` does **not** unmount Vue components, so `onBeforeUnmount` cleanup never runs.
### Fix Focus Areas
- packages/super-editor/src/editors/v1/components/toolbar/SdTooltip.test.js[6-11]
- packages/super-editor/src/editors/v1/components/toolbar/SdTooltip.test.js[74-93]
### Suggested fix
- Track mounted wrappers and unmount them in `afterEach` (e.g., push each `wrapper` returned by `mount()` into an array and `unmount()` them during cleanup).
- Alternatively, change `mountAndOpen()` to return `{ wrapper, content }` and have each test call `wrapper.unmount()` in a `finally` block.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
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.
Summary
SdTooltip.vue'supdatePosition()clamped the tooltip horizontally against the viewport but always placed it above the trigger (triggerRect.top - contentHeight - 10), with no vertical bounds check.triggerRect.bottom + offset) instead. Placement is exposed asdata-placement="top" | "bottom"on.sd-tooltip-content, which the component's own CSS uses to flip the arrow anchor and the transition'stransform-originso the tooltip still points at and grows from the trigger.data-placementas a small styling contract on the tooltip content element (same pattern asfloating-ui/Radix
data-side). Pre-existing and untouched: the arrow is centered on the tooltip, so it can miss the trigger when the tooltip is horizontally clamped at the far left/right viewport edges.Before / After
Toolbar flush with the top of the viewport, hovering the first toolbar button:
Test plan
SdTooltip.test.js(positioningblock): tooltip stays fully inside the viewport and clear of the triggerwhen the trigger is flush with the top; still renders fully above when there is room;
data-placementmarks the side so the arrow points at the trigger. Verified the flush-top test fails without the fix (tooltip attop: -44px) and passes with it.SdTooltipauto-hide test unchanged and passing.@superdoc/super-editortest suite: 1218 files / 17210 tests passed.