Move Traffic Boost actions from highlight markup to separate container#3460
Move Traffic Boost actions from highlight markup to separate container#3460
Conversation
…ll needs some positioning work
…size, fix captured action bar mouse events
📝 Walkthrough""" WalkthroughThis update refactors the Traffic Boost preview UI and highlight logic. It improves text selection handling, normalizes triple-click selection, simplifies the highlight DOM structure, repositions the actions bar above highlights, removes tooltip and hint interactivity, and deletes the unused Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Iframe
participant HighlightLogic
participant ActionsBar
User->>Iframe: Triple-clicks or selects text
Iframe->>HighlightLogic: Detects selection
HighlightLogic->>HighlightLogic: Normalize selection range
HighlightLogic->>HighlightLogic: Extract selected text accurately
HighlightLogic->>HighlightLogic: Insert highlight span
HighlightLogic->>ActionsBar: Position actions bar above highlight
User->>ActionsBar: Interacts with actions bar (if desired)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
564-571: Overlay can be mis-aligned after range expansion.
updatePosition()relies on thenormalizedRangecaptured beforeexpandToLinkNode/expandToWordBoundary.
If either helper replaces the selection with a newRange, the overlay rectangle no longer matches the actual selection, leading to visible offset.Compute the rect from the current selection instead:
- const rect = normalizedRange.getBoundingClientRect(); + const rect = iframeDocument.getSelection()! + .getRangeAt( 0 ) + .getBoundingClientRect();(or recompute
normalizedRangeafter expansion).
🧹 Nitpick comments (2)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-actions.tsx (1)
138-144: Cursor suggests interactivity where none exists.
.traffic-boost-preview-actions-hintstill receivescursor: help;but the tooltip functionality was removed, so the element is no longer interactive. Either drop the pointer-style cursor or add an actualtitle/tooltip for consistency and accessibility.src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
532-544: Inefficient range cloning loop.Cloning every range into a temporary container is correct for preserving entities, but using a plain loop is simpler & faster:
- for ( let i = 0; i < docSelection.rangeCount; i++ ) { - selectionContainer.append( docSelection.getRangeAt( i ).cloneContents() ); - } + Array.from( { length: docSelection.rangeCount } ).forEach( ( _, i ) => + selectionContainer.append( docSelection.getRangeAt( i ).cloneContents() ) + );Minor, but keeps the code compact.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (5)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-actions.tsx(2 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsx(2 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(8 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts(10 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-resize.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-resize.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-actions.tsxsrc/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsxsrc/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsxsrc/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts
🧬 Code Graph Analysis (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-draggable.ts (1)
DRAG_MARGIN_PX(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: E2E against WordPress latest
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsx
Outdated
Show resolved
Hide resolved
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts
Show resolved
Hide resolved
acicovic
added
Feature: PCI
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (1)
520-537: Global cleanup is invoked for every highlight removal.Calling
window.wpParselyTrafficBoostCleanupActionsBar()inremoveAndCleanmeans removing one highlight tears down the toolbar even when another highlight still needs it (rare but possible during batch operations).Add a guard: only call if the target highlight owns the current toolbar, or delegate this responsibility to higher-level logic.
🧹 Nitpick comments (4)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (4)
171-180: Potential first-paint flash & redundant inline positioning.The CSS gives the container an initial
top/left: ${ DRAG_MARGIN_PX }px;even though JS immediately overwritestop(and oftenleft).
Because opacity is0, the bar is invisible, so the inline coordinates aren’t needed and create dead code.- top: ${ DRAG_MARGIN_PX }px; - left: ${ DRAG_MARGIN_PX }px; + /* let JS set coordinates – avoids unnecessary style churn. */Removing the hard-coded offsets avoids needless style recalculations and keeps the stylesheet declarative.
186-193: Alignment classes duplicate inline styles.
.align-leftsets the sameleftvalue that already lives in the base rule.
Either rely on the base rule or switch the base rule toleft: autoso the modifier actually modifies.This small redundancy can be confusing when debugging.
361-365: Guard clause can be simplified.
existingActions && window.wpParselyTrafficBoostCleanupActionsBaris fine, butwindow.wpParselyTrafficBoostCleanupActionsBar?.()is shorter and avoids the extra DOM query.- const existingActions = iframeDocument.querySelector( '.parsely-traffic-boost-actions-container' ); - if ( existingActions && window.wpParselyTrafficBoostCleanupActionsBar ) { - window.wpParselyTrafficBoostCleanupActionsBar(); - } + window.wpParselyTrafficBoostCleanupActionsBar?.();
427-435: Scope the query to avoid false positives.
iframeDocument.querySelector('.traffic-boost-preview-actions')scans the entire DOM and could collide with other content that happens to carry the same class. Query insideactionsContainerinstead:- const renderedActionsBar = iframeDocument.querySelector( '.traffic-boost-preview-actions' ) as HTMLElement; + const renderedActionsBar = actionsContainer.querySelector( '.traffic-boost-preview-actions' ) as HTMLElement;This is faster and more robust.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (3)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsx(2 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(9 hunks)src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/content-helper/dashboard-page/pages/traffic-boost/preview/components/preview-iframe.tsx
- src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts
🧠 Learnings (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (1)
Learnt from: alecgeatches
PR: Parsely/wp-parsely#3460
File: src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts:421-480
Timestamp: 2025-06-16T21:30:45.103Z
Learning: In iframe highlight positioning systems, when using `position: absolute` with document-relative coordinates calculated as `element.getBoundingClientRect().top + document.scrollTop`, the positioned element will naturally move with scroll events without requiring additional scroll event listeners, as both the target element and positioned element exist in the same document coordinate system.
🧬 Code Graph Analysis (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-draggable.ts (1)
DRAG_MARGIN_PX(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (1)
485-486: Throttle interval may be too coarse for high-density layouts.A 100 ms window is perceptible on aggressive resize (e.g. split-screen drag). Consider 50 ms to feel snappier without hurting performance.
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts
Show resolved
Hide resolved
|
@alecgeatches, I've pushed commits to improve comments and also restored the exception handler in I'll be now testing this to check whether it solves the 3 issues mentioned. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
358-375: Cleanup logic leaks scroll listeners on every new selection.When a new selection is made you remove the previous highlight (
existingHighlight.remove()), yet thescrollHandleradded a few lines below remains attached because itscleanup()closure lives in the previous invocation ofhandleSelection. Re-selecting text repeatedly will therefore accumulatescrolllisteners on both the iframe and the parent window.Consider persisting the
cleanupfunction on the highlight element (or in aWeakMap) so you can call it before removing the node, or hoist a single scroll listener that simply checks whether a highlight is present.- existingHighlight.remove(); + existingHighlight.dispatchEvent(new Event('parsely-cleanup')); // triggers stored cleanupor
// after creating highlight highlight.addEventListener('parsely-cleanup', cleanup);Make sure to invoke
cleanup()before removing the element to detach the listeners and avoid memory / performance degradation.Also applies to: 491-507
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
🧠 Learnings (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
Learnt from: alecgeatches
PR: Parsely/wp-parsely#3460
File: src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts:421-480
Timestamp: 2025-06-16T21:30:45.103Z
Learning: In iframe highlight positioning systems, when using `position: absolute` with document-relative coordinates calculated as `element.getBoundingClientRect().top + document.scrollTop`, the positioned element will naturally move with scroll events without requiring additional scroll event listeners, as both the target element and positioned element exist in the same document coordinate system.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
36-38: z-index bump looks safe.Raising the highlight from
1000to1010keeps it above the admin toolbar (z-index: 1000) while still below fixed WP modals (100000). No issues spotted.
| while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/g.test( startText[ startOffset - 1 ] ) ) { | ||
| startOffset--; | ||
| } | ||
| } | ||
|
|
||
| // Find word boundary at end. | ||
| const endNode = range.endContainer as Text; | ||
| const initialEnd = range.endOffset; | ||
| let endOffset = range.endOffset; | ||
|
|
||
| if ( endNode.nodeType === Node.TEXT_NODE ) { | ||
| const endText = endNode.textContent ?? ''; | ||
| while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/g.test( endText[ endOffset ] ) ) { | ||
| endOffset++; | ||
| } |
There was a problem hiding this comment.
Regex with global (g) flag inside .test() causes stateful bugs.
RegExp.prototype.test with the g flag mutates lastIndex; consecutive calls can therefore skip characters and break the boundary-scan loop.
- while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/g.test( startText[ startOffset - 1 ] ) ) {
+ while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/.test( startText[ startOffset - 1 ] ) ) {
…
- while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/g.test( endText[ endOffset ] ) ) {
+ while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/.test( endText[ endOffset ] ) ) {Removing g keeps the regex stateless and deterministic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/g.test( startText[ startOffset - 1 ] ) ) { | |
| startOffset--; | |
| } | |
| } | |
| // Find word boundary at end. | |
| const endNode = range.endContainer as Text; | |
| const initialEnd = range.endOffset; | |
| let endOffset = range.endOffset; | |
| if ( endNode.nodeType === Node.TEXT_NODE ) { | |
| const endText = endNode.textContent ?? ''; | |
| while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/g.test( endText[ endOffset ] ) ) { | |
| endOffset++; | |
| } | |
| while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/.test( startText[ startOffset - 1 ] ) ) { | |
| startOffset--; | |
| } | |
| } | |
| // Find word boundary at end. | |
| const endNode = range.endContainer as Text; | |
| const initialEnd = range.endOffset; | |
| let endOffset = range.endOffset; | |
| if ( endNode.nodeType === Node.TEXT_NODE ) { | |
| const endText = endNode.textContent ?? ''; | |
| while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/.test( endText[ endOffset ] ) ) { | |
| endOffset++; | |
| } |
🤖 Prompt for AI Agents
In
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
around lines 656 to 670, the regular expressions used in the while loops for
detecting word boundaries include the global flag 'g', which causes stateful
behavior and can skip characters due to the mutation of lastIndex in
RegExp.prototype.test. To fix this, remove the 'g' flag from both regular
expressions so that the test method behaves statelessly and the loops correctly
identify word boundaries without skipping characters.
There was a problem hiding this comment.
This may be true, but for now it seems to work as-is so I'm not going to modify this now.
There was a problem hiding this comment.
@alecgeatches Understood! You're right that it's working as expected currently. If you'd like to track this for a future cleanup, I can create an issue to document the potential regex stateful behavior concern, but no worries if you prefer to leave it as-is for now.
...tent-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
32-38: Popover becomes un-clickable due to inheritedpointer-events: none.
pointer-events: none;on.parsely-traffic-boost-highlightdisables all mouse interaction for the element and its descendants, which includes the popover container and the buttons inside it. This prevents users from clicking “Use as Link Text / Replace Link”.Proposed quick fix – keep the overlay transparent to selections, but re-enable events only for the popover:
.parsely-traffic-boost-highlight { position: absolute; /* Allow overlay itself to ignore clicks … */ pointer-events: none; z-index: 1010; transition: all 0.15s ease-out; } .parsely-traffic-boost-popover-container { position: absolute; + /* … but re-enable interaction for the popover. */ + pointer-events: auto; left: 50%; bottom: 100%; transform: translateX(-50%);
♻️ Duplicate comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
685-699: Regex still uses thegflag inside.test()– inherits previous issue.Comment already raised in an earlier review; the stateful
lastIndexcontinues to cause skipped characters on consecutive calls. Please drop thegflag.
🧹 Nitpick comments (2)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (2)
458-475: Scroll listeners are probably redundant and unthrottled.Because the highlight’s absolute coordinates are recomputed from
getBoundingClientRect()plusscrollY, its position already tracks scrolling without any extra work (see internal learning note on iframe positioning). The added scroll handlers:
- Fire on every scroll event in both the iframe and the parent window.
- Have no throttling apart from a single
requestAnimationFrame, which can still overwhelm low-end devices during rapid scrolling.Unless you have confirmed visual glitches without them, consider deleting the listeners and relying on normal document flow, or at least debounce them.
617-626: Minor: fallback for nodes without aparentElement.
node.parentElementisnullforDocumentFragmentand certain text nodes; calling.closest()onnullis safe, but you silently skip those nodes. Usingnode.parentNode as Elementwould cover both cases. Not critical, just a consideration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
🧠 Learnings (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
Learnt from: alecgeatches
PR: Parsely/wp-parsely#3460
File: src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts:421-480
Timestamp: 2025-06-16T21:30:45.103Z
Learning: In iframe highlight positioning systems, when using `position: absolute` with document-relative coordinates calculated as `element.getBoundingClientRect().top + document.scrollTop`, the positioned element will naturally move with scroll events without requiring additional scroll event listeners, as both the target element and positioned element exist in the same document coordinate system.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
...tent-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
448-451: Horizontal scroll not accounted for in overlay positioning.
leftusesrect.left(viewport-relative) without addingscrollX, so the highlight drifts when the iframe content is horizontally scrolled.If you keep manual positioning, mirror the vertical logic:
- highlight.style.left = `${ rect.left }px`; + const scrollX = iframeDocument.defaultView?.scrollX ?? 0; + highlight.style.left = `${ rect.left + scrollX }px`;
♻️ Duplicate comments (2)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (2)
588-590:document.createRange()built in the wrong document – still breaks iframe selections.
normalizeRangecreates a range withdocument.createRange(), which belongs to the parent document.
When the selection originates inside the iframe, every triple-click throwsWRONG_DOCUMENT_ERRin all major browsers.This was flagged in a previous review and remains unresolved.
Replace the call with the owner document of the start node:- const newRange = document.createRange(); + const newRange = ( range.startContainer.ownerDocument || document ).createRange();
690-703: Global-flag regex in.test()is still stateful and skips characters.The two
whileloops use/…/g.test(...).
Because the pattern has thegflag,RegExp.prototype.testmutateslastIndex; consecutive calls falsely returnfalse, making the boundary scan unreliable.Drop the
gflag to keep the regex stateless:- while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/g.test( startText[ startOffset - 1 ] ) ) { + while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/.test( startText[ startOffset - 1 ] ) ) { … - while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/g.test( endText[ endOffset ] ) ) { + while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/.test( endText[ endOffset ] ) ) {
🧹 Nitpick comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
445-463: Scroll listeners & manual repositioning are redundant and incomplete.Given the highlight overlay is
position:absoluteinside the same document as the selected text, its coordinates already move with scrolling (see learning note from PR #3460). The extrascrolllisteners:
- Perform an unnecessary
getBoundingClientRect/layout thrash on every scroll.- Adjust only
scrollY, ignoring horizontal scroll (scrollX), which can mis-align the overlay.Unless you later detach the overlay into a fixed-position layer, you can delete the listener block and remove the
+ scrollYoffset – the element will track naturally.- const rect = normalizedRange.getBoundingClientRect(); - const scrollY = iframeDocument.defaultView?.scrollY ?? 0; - highlight.style.top = `${ rect.top + scrollY }px`; + const rect = normalizedRange.getBoundingClientRect(); + highlight.style.top = `${ rect.top }px`; … - iframeDocument.addEventListener( 'scroll', scrollHandler, { passive: true } ); - window.addEventListener( 'scroll', scrollHandler, { passive: true } ); + // No explicit scroll handler required.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
🧠 Learnings (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
Learnt from: alecgeatches
PR: Parsely/wp-parsely#3460
File: src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts:421-480
Timestamp: 2025-06-16T21:30:45.103Z
Learning: In iframe highlight positioning systems, when using `position: absolute` with document-relative coordinates calculated as `element.getBoundingClientRect().top + document.scrollTop`, the positioned element will naturally move with scroll events without requiring additional scroll event listeners, as both the target element and positioned element exist in the same document coordinate system.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
691-705: Regex with global (g) flag inside.test()is still stateful – removeg.This was flagged in an earlier review and remains unresolved.
RegExp.prototype.testmutateslastIndexwhen the pattern hasg, leading to missed characters on subsequent iterations.-while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/g.test( startText[ startOffset - 1 ] ) ) { +while ( startOffset > 0 && /[^\s.,!?;:'"’)\]}]/.test( startText[ startOffset - 1 ] ) ) { … -while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/g.test( endText[ endOffset ] ) ) { +while ( endOffset < endText.length && /[^\s.,!?;:'"’([{]/.test( endText[ endOffset ] ) ) {
🧹 Nitpick comments (4)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (4)
349-358: Middle ranges are not validated – multi-range selections can still straddle paragraphs.The logic checks only the first and last ranges for paragraph alignment. If
docSelectioncontains more than two ranges, any range in the middle could still cross paragraph boundaries and slip through this guard.Consider iterating over all ranges (
for … rangeCount) and ensuring every range’s start/end container resolves to the same paragraph element.
458-463: Scroll listeners may be unnecessary – absolute positioning already follows scroll.Because
highlightand the target text live in the same document, coordinates fromgetBoundingClientRect()already account for scrolling; the element will move naturally. Listening to both the iframe and parent window scroll events on every highlight can introduce unnecessary re-flows and event overhead.Re-test without these listeners; if behaviour is unchanged, the listeners (and cleanup code) can be dropped.
489-495: Debounce fires twice per event due toleading: true, trailing: true.
@wordpress/compose’sdebouncewill invoke the wrapped function on both the leading and trailing edge, resulting in twohandleSelectionexecutions for each selection change. If you only need one call, set eitherleadingortrailingtotrue.-const handleSelectionChange = debounce( handleSelection, 300, { - leading: true, - trailing: true, -} ); +const handleSelectionChange = debounce( handleSelection, 300, { trailing: true } );
566-596: Create the newRangein the owner document to avoidWRONG_DOCUMENT_ERR.
currentDocument.createRange()is safe only whencurrentDocumentis guaranteed to be the same asstartParagraph.ownerDocument. PassingiframeDocumentfrom the caller works today but is fragile if this helper is reused elsewhere.-const newRange = currentDocument.createRange(); +const newRange = ( startParagraph.ownerDocument || currentDocument ).createRange();This mirrors the fix applied elsewhere in the codebase and prevents cross-document exceptions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx
🧠 Learnings (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/components/text-selection-tooltip.tsx (1)
Learnt from: alecgeatches
PR: Parsely/wp-parsely#3460
File: src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts:421-480
Timestamp: 2025-06-16T21:30:45.103Z
Learning: In iframe highlight positioning systems, when using `position: absolute` with document-relative coordinates calculated as `element.getBoundingClientRect().top + document.scrollTop`, the positioned element will naturally move with scroll events without requiring additional scroll event listeners, as both the target element and positioned element exist in the same document coordinate system.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: E2E against WordPress latest
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (5)
20-22: Consider tightening the global namespace.
window.wpParselyTrafficBoostCleanupActionsBaris already reasonably namespaced, but it is still a mutable global.
For even lower collision-risk you could attach it towindow.wpParsely = { … }or wrap it in aSymbol. Purely optional.
171-250: Hard-coded colours & spacing – prefer design tokens / CSS variables.The new rules embed literal values like
#3858E9,#1e1e1e, and35pxoffsets.
Using WP admin CSS variables (e.g.var(--wp-admin-theme-color)), or defining local custom properties, makes future theming easier and aligns with WordPress standards.
Example:- outline: 2px solid #3858E9; + outline: 2px solid var(--tb-highlight-color, #3858E9);Same applies to the fixed
35/48/36pixel constants.
408-419: Docblock wording & early cancellation.
The comment says “Sets up the actions bar cleanup function”, but the function itself performs the cleanup.
Rename to “Cleans up the actions bar” (or adjust the description) for clarity.The cancellation block is 👍 – it prevents the throttled callback from firing after unmount.
431-483: Re-addingfade-inon every resize may cause flicker.
positionActionsBarcallsclassList.add('fade-in')unconditionally.
After the first call the class is already present, so subsequent additions are no-ops in most browsers, but Safari will restart the transition, producing a subtle opacity flash.Guard once:
- actionsContainer.classList.add( 'fade-in' ); + if ( ! actionsContainer.classList.contains( 'fade-in' ) ) { + actionsContainer.classList.add( 'fade-in' ); + }
485-491: Timeout-based scroll settling is brittle.Waiting a fixed
400 msassumes the iframe finishes auto-scrolling in that window.
Consider:const waitForScroll = () => new Promise( r => { const done = () => { iframeDocument.removeEventListener('scroll', done); r(); }; iframeDocument.addEventListener('scroll', done, { once: true, passive: true }); }); await waitForScroll(); positionActionsBar();or
requestAnimationFramepolling untilscrollTopstabilises.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/dashboard-page.asset.phpis excluded by!build/**build/content-helper/dashboard-page.jsis excluded by!build/**
📒 Files selected for processing (1)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,jsx}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the code to ensure it is well-structured and adheres to best ...
**/*.{js,ts,tsx,jsx}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the code to ensure it is well-structured and adheres to best practices.
- Verify compliance with WordPress coding standards.
- Ensure the code is well-documented.
- Check for security vulnerabilities and confirm the code is secure.
- Optimize the code for performance, removing any unnecessary elements.
- Validate JSDoc comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Confirm every JSDoc comment includes a @SInCE tag indicating the next version of the plugin to include the code.
- Guarantee compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: E2E against WordPress latest
🔇 Additional comments (4)
src/content-helper/dashboard-page/pages/traffic-boost/preview/hooks/use-iframe-highlight.ts (4)
10-10: Import looks correct.
throttleis now imported exactly where it is used, eliminating an implicit global dependency.
Nothing else to flag.
361-364: Previous actions bar is cleaned before rendering the new one.Good defensive check – prevents duplicate toolbars and dangling listeners.
494-496: Resize listener throttling is correctly implemented.The handler is throttled and properly removed in the cleanup routine – well done.
526-532: Cleanup is invoked for every highlight removal.Calling
window.wpParselyTrafficBoostCleanupActionsBar()here guarantees no orphaned toolbar even when multiple highlights are removed in quick succession. Looks good.
2 participants
Description
Move the Traffic Boost actions bar out of the highlight markup for a more consistent experience across browsers and page markup. This required some changes to positioning calculations. Additionally, this PR adds:
Updated selection hint visuals on the right-hand side of the bar:
Triple-click fix for selecting entire paragraphs, fixes PCH Traffic Boost: Allow triple-clicking to select a highlight #3458.
Adds margin between the actions toolbar and highlighted text, should fix PCH Traffic Boost: Popup toolbar placement #3457:
CSS changes that fix PCH Traffic Boost: 'Highlight to Customize' cursor #3456.
Motivation and context
See the "Actions bar markup as a child of highlight span" section in #3445 for a summary of issues from embedding the actions bar with the highlight markup:
After the prior PR was merged, we also discovered additional problems highlighting in Chrome due to the embedded highlight markup and other triple-click selection issues.
Separating the actions bar from the highlight markup makes
Selectionranges are much more consistent.How has this been tested?
Summary by CodeRabbit