Skip to content

Add replace-with-answer button to selection tools#996

Open
sebastianelsner wants to merge 5 commits into
ChatGPTBox-dev:masterfrom
sebastianelsner:feature/selection-tools-replace
Open

Add replace-with-answer button to selection tools#996
sebastianelsner wants to merge 5 commits into
ChatGPTBox-dev:masterfrom
sebastianelsner:feature/selection-tools-replace

Conversation

@sebastianelsner

@sebastianelsner sebastianelsner commented Jul 8, 2026

Copy link
Copy Markdown

Summary

Adds a Replace button to answers in the selection-tools floating toolbar, so the AI response (e.g. a polished or translated version) can be written back over the originally selected text in one click — no more manual copy & paste when polishing text you are writing in an input, textarea, or rich-text editor.

grafik

How it works

  • When the selection toolbar is triggered, the content script captures the editable target of the selection (captureEditableSelection) — a text field (input/textarea) with its selection offsets, or a cloned Range inside a contenteditable region. This happens before the selection is lost by interacting with the toolbar.
  • Completed answers in the floating toolbar then show a Replace button (next to Copy). Clicking it calls replaceCapturedSelection, which writes the answer text over the original selection. A ✓/✗ icon gives success/failure feedback (same pattern as CopyButton).
  • The button only appears when the selection was inside an editable element. Static page content is intentionally not replaceable — replacing arbitrary page DOM is destructive and fragile, and the real use case is text you are writing.

Safety / security considerations

  • No HTML injection: the answer is inserted as plain text only (.value / text nodes / execCommand('insertText')), never parsed as HTML.
  • Stale-capture guard: before replacing, the original text is re-verified at the captured location (offsets for text fields, range.toString() for contenteditable). If the content changed in the meantime, nothing is overwritten and the button shows a failure state.
  • Undo works: document.execCommand('insertText') is preferred so the native undo history keeps working; a fallback sets the value via the prototype setter and dispatches an input event so pages with controlled inputs (React and similar) observe the change.

Containment

  • New util src/utils/selection-replace.mjs + new component src/components/ReplaceButton/; everything else is optional-prop threading (capturedSelectionFloatingToolbar, onReplaceContentConversationCard, onReplaceConversationItem).
  • Other users of ConversationCard/FloatingToolbar (popup, independent panel, site adapters, menu tools) are unaffected — the capture is only wired up in the selection-tools path (mouse and touch).

i18n

  • Added keys Replace Selection and Replace Failed to src/_locales/en/main.json and propagated translations to all other locales (de, es, fr, in, it, ja, ko, pt, ru, tr, zh-hans, zh-hant). Native speakers, please feel free to correct.

Validation

  • npm test: 708 tests pass, including 19 new unit tests for selection-replace.mjs (capture and replace for both text fields and contenteditable, execCommand and fallback paths, stale/disconnected guards).
  • npm run lint and npm run pretty: clean.
  • npm run build: all build variants and artifacts produced successfully.
  • Manual browser smoke tests: not yet performed in my environment (headless); happy to add screenshots — the UI change is a single icon button in the existing answer header.

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary

  • New Features

    • Added a “Replace Selection” action to answer cards with localized tooltip and visible success/failure feedback.
    • Enhanced selection handling so the replace action works from both selection and context-menu flows.
  • Bug Fixes

    • Improved capturing and replacing text in editable fields and rich content, including safer behavior when selection state is stale or replacement isn’t possible.
  • Tests

    • Added unit tests covering selection capture and replacement for text inputs and contenteditable content.

When the selection toolbar is triggered on text inside an editable
target (input, textarea, or contenteditable element), the editable
selection is captured before it gets lost by interacting with the
toolbar. Completed answers in the floating toolbar then show a Replace
button next to Copy that writes the answer back over the originally
selected text, e.g. to apply a polished or translated version in place.

The replacement verifies the original text is still present before
writing (stale captures never overwrite changed content), inserts plain
text only (never parsed as HTML), prefers execCommand('insertText') so
native undo keeps working, and falls back to setting the value through
the prototype setter and dispatching an input event so pages with
controlled inputs (React and similar) observe the change.

Static page content is intentionally not replaceable, and other users
of ConversationCard and FloatingToolbar (popup, independent panel, site
adapters) are unaffected since the capture is only wired up in the
selection tools path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c985e6ce-5427-4c8c-a1df-c7e2703addd2

📥 Commits

Reviewing files that changed from the base of the PR and between 0fb4fd7 and 25fbd1e.

📒 Files selected for processing (3)
  • src/content-script/index.jsx
  • src/utils/selection-replace.mjs
  • tests/unit/utils/selection-replace.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/utils/selection-replace.mjs
  • src/content-script/index.jsx
  • tests/unit/utils/selection-replace.test.mjs

📝 Walkthrough

Walkthrough

Adds a replace-selection flow that captures editable text, threads the captured selection through the toolbar and conversation UI, and replaces it with new content using dedicated utilities. It also adds tests and new locale strings.

Changes

Replace Selection Feature

Layer / File(s) Summary
Selection capture/replace utility functions
src/utils/selection-replace.mjs, src/utils/index.mjs
New selection capture and replacement utilities for text fields and contenteditable elements, re-exported through the utils index.
Content script and FloatingToolbar wiring
src/content-script/index.jsx, src/components/FloatingToolbar/index.jsx
Captures editable selection in mouse, touch, and context-menu flows and passes capturedSelection into the toolbar replacement path.
ReplaceButton and conversation UI integration
src/components/ReplaceButton/index.jsx, src/components/ConversationItem/index.jsx, src/components/ConversationCard/index.jsx
Adds ReplaceButton and wires replace callbacks through conversation rendering for completed answers.
Unit tests and locale strings
tests/unit/utils/selection-replace.test.mjs, src/_locales/*/main.json
Adds coverage for capture and replacement behavior, plus translations for the new replace action and failure message.

Estimated code review effort: 4 (Complex) | ~50 minutes

Suggested labels: Review effort 4/5

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: adding a replace action button to the selection tools toolbar.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

qodo-code-review Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add Replace button to selection-tools answers to write back into editable fields

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Capture editable selections (text fields/contenteditable) before the toolbar steals focus.
• Add a Replace action on completed answers to overwrite the captured selection safely.
• Add i18n strings and unit tests covering execCommand/fallback and stale-selection guards.
Diagram

graph TD
  CS["content-script selection tools"] --> CAP["captureEditableSelection()"] --> FT["FloatingToolbar"] --> CC["ConversationCard/Item"] --> RB["ReplaceButton"] --> UTIL["replaceCapturedSelection()"] --> DOM["Editable DOM target"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use setRangeText() for inputs/textareas
  • ➕ Native API for replacing selected text in text fields
  • ➕ Often integrates cleanly with selection and value updates without execCommand
  • ➖ Does not help for contenteditable
  • ➖ Controlled inputs may still require careful event dispatching; cross-browser quirks remain
2. Use beforeinput/InputEvent-driven insertion for contenteditable
  • ➕ More standards-forward than execCommand in modern browsers
  • ➕ Potentially better integration with editors that intercept input events
  • ➖ More complex compatibility surface across browsers/editors
  • ➖ May still require fallbacks; harder to test deterministically

Recommendation: The PR’s approach is solid for an extension context: capture-then-replace avoids focus/selection loss, replacement is plain-text only, and stale-capture verification prevents unintended overwrites. Keeping execCommand('insertText') as the preferred path preserves undo where supported, with a pragmatic fallback for controlled inputs.

Files changed (21) +695 / -17

Enhancement (7) +280 / -17
index.jsxThread onReplaceContent to answers and strip trailing <hr/> +6/-0

Thread onReplaceContent to answers and strip trailing <hr/>

• Adds an optional onReplaceContent prop and wires it to answer items only when the answer is done. Reuses the same trailing <hr/> trimming behavior used by Copy to avoid inserting UI separators into the replaced text.

src/components/ConversationCard/index.jsx

index.jsxRender ReplaceButton in answer header when onReplace is provided +4/-1

Render ReplaceButton in answer header when onReplace is provided

• Introduces ReplaceButton next to Copy for answer items and adds a new onReplace prop. The button is gated by descName (answer context) and presence of an onReplace handler.

src/components/ConversationItem/index.jsx

index.jsxWire capturedSelection into Replace flow via replaceCapturedSelection() +12/-1

Wire capturedSelection into Replace flow via replaceCapturedSelection()

• Extends FloatingToolbar to accept a capturedSelection prop and maps it to ConversationCard.onReplaceContent. Replacement is performed via the shared utility so selection-tools can safely write into editable targets.

src/components/FloatingToolbar/index.jsx

index.jsxAdd ReplaceButton component with success/failure feedback +49/-0

Add ReplaceButton component with success/failure feedback

• Adds a new icon button that invokes onReplace and shows transient ✓/✗ feedback with an i18n tooltip. Cleans up timers on unmount to avoid leaking timeouts.

src/components/ReplaceButton/index.jsx

index.jsxCapture editable selection before showing selection-tools (mouse/touch/menu) +39/-15

Capture editable selection before showing selection-tools (mouse/touch/menu)

• Captures the editable selection descriptor before toolbar interaction can clear it, passes it into FloatingToolbar, and logs it in debug output. Adds a Firefox-specific fallback to derive selection text from captured text-field selections when window.getSelection() doesn’t expose them, and captures selection early for context-menu initiated selection tools.

src/content-script/index.jsx

index.mjsExport selection replacement helpers from utils barrel +1/-0

Export selection replacement helpers from utils barrel

• Re-exports the new selection-replace module so UI code can import replaceCapturedSelection from the central utils entry point.

src/utils/index.mjs

selection-replace.mjsAdd captureEditableSelection() and replaceCapturedSelection() with safety guards +169/-0

Add captureEditableSelection() and replaceCapturedSelection() with safety guards

• Implements capturing of either text-field offsets or cloned contenteditable ranges. Replacement verifies the original text to prevent stale overwrites, inserts plain text only, prefers execCommand('insertText') for undo support, and falls back to prototype value-setting plus input event dispatch for controlled inputs.

src/utils/selection-replace.mjs

Tests (1) +389 / -0
selection-replace.test.mjsAdd unit tests for selection capture and replacement across editable types +389/-0

Add unit tests for selection capture and replacement across editable types

• Adds thorough unit coverage for text-field and contenteditable capture/replace paths, including execCommand success/failure, stale/disconnected guards, and input event dispatch behavior.

tests/unit/utils/selection-replace.test.mjs

Other (13) +26 / -0
main.jsonAdd Replace Selection / Replace Failed i18n strings (de) +2/-0

Add Replace Selection / Replace Failed i18n strings (de)

• Adds German translations for the new Replace button label and failure tooltip/state.

src/_locales/de/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (en) +2/-0

Add Replace Selection / Replace Failed i18n strings (en)

• Adds the English source strings for the Replace action and failure message.

src/_locales/en/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (es) +2/-0

Add Replace Selection / Replace Failed i18n strings (es)

• Adds Spanish translations for the Replace action and failure state messaging.

src/_locales/es/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (fr) +2/-0

Add Replace Selection / Replace Failed i18n strings (fr)

• Adds French translations for the Replace action label and failure tooltip/state.

src/_locales/fr/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (in) +2/-0

Add Replace Selection / Replace Failed i18n strings (in)

• Adds Indonesian translations for the Replace action and failure state messaging.

src/_locales/in/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (it) +2/-0

Add Replace Selection / Replace Failed i18n strings (it)

• Adds Italian translations for the Replace action label and failure tooltip/state.

src/_locales/it/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (ja) +2/-0

Add Replace Selection / Replace Failed i18n strings (ja)

• Adds Japanese translations for the Replace action and failure tooltip/state.

src/_locales/ja/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (ko) +2/-0

Add Replace Selection / Replace Failed i18n strings (ko)

• Adds Korean translations for the Replace action and failure tooltip/state.

src/_locales/ko/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (pt) +2/-0

Add Replace Selection / Replace Failed i18n strings (pt)

• Adds Portuguese translations for the Replace action and failure tooltip/state.

src/_locales/pt/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (ru) +2/-0

Add Replace Selection / Replace Failed i18n strings (ru)

• Adds Russian translations for the Replace action and failure tooltip/state.

src/_locales/ru/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (tr) +2/-0

Add Replace Selection / Replace Failed i18n strings (tr)

• Adds Turkish translations for the Replace action and failure tooltip/state.

src/_locales/tr/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (zh-hans) +2/-0

Add Replace Selection / Replace Failed i18n strings (zh-hans)

• Adds Simplified Chinese translations for the Replace action and failure tooltip/state.

src/_locales/zh-hans/main.json

main.jsonAdd Replace Selection / Replace Failed i18n strings (zh-hant) +2/-0

Add Replace Selection / Replace Failed i18n strings (zh-hant)

• Adds Traditional Chinese translations for the Replace action and failure tooltip/state.

src/_locales/zh-hant/main.json

@sebastianelsner sebastianelsner marked this pull request as draft July 8, 2026 22:51

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a feature to replace captured editable text selections (such as text fields and contenteditable regions) with generated or translated content, including a new ReplaceButton component, updated localization files, and comprehensive unit tests. The code review feedback suggests several important improvements: traversing the DOM to find the top-most contenteditable ancestor to ensure focus works correctly, re-applying the collapsed range to the active selection in the contenteditable fallback path to update the cursor position, and using a useEffect hook in ReplaceButton to clear the status timeout on unmount to prevent memory leaks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +17 to +21
const findContentEditableElement = (node) => {
if (!node) return null
const element = node.nodeType === 3 ? node.parentElement : node
return element && element.isContentEditable ? element : null
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When the selection is inside a child element of a contenteditable region (e.g., a <b> or <p> tag), findContentEditableElement returns that child element. However, calling .focus() on non-focusable elements like <b> does not actually focus the editing host, which can cause subsequent doc.execCommand calls to fail or behave unexpectedly.

Traversing up the DOM tree to find the top-most contenteditable ancestor (the editing host) ensures that .focus() is called on a focusable element.

const findContentEditableElement = (node) => {
  if (!node) return null
  let element = node.nodeType === 3 ? node.parentElement : node
  if (!element || !element.isContentEditable) return null
  while (element.parentElement && element.parentElement.isContentEditable) {
    element = element.parentElement
  }
  return element
}

Comment on lines +131 to +134
range.deleteContents()
range.insertNode(doc.createTextNode(text))
range.collapse(false)
dispatchInputEvent(element)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In the fallback path for contenteditable replacement, range.collapse(false) is called to collapse the range to the end of the inserted text. However, this only updates the Range object itself and does not update the browser's actual selection/cursor.

To ensure the cursor is correctly placed at the end of the replaced text so the user can continue typing seamlessly, we should re-apply the collapsed range to the active selection.

  range.deleteContents()
  const textNode = doc.createTextNode(text)
  range.insertNode(textNode)
  range.collapse(false)
  if (selection) {
    selection.removeAllRanges()
    selection.addRange(range)
  }
  dispatchInputEvent(element)

Comment thread src/components/ReplaceButton/index.jsx Outdated
Comment on lines +15 to +24
const timeoutRef = useRef(null)

const onClick = () => {
setStatus(onReplace() ? 'replaced' : 'failed')
if (timeoutRef.current) clearTimeout(timeoutRef.current)
timeoutRef.current = setTimeout(() => {
setStatus('idle')
timeoutRef.current = null
}, 800)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add a useEffect hook to clear the pending timeout when the component unmounts. This prevents memory leaks and potential state updates on unmounted components if the button is unmounted while the success/failure status timeout is still active.

  const timeoutRef = useRef(null)

  useEffect(() => {
    return () => {
      if (timeoutRef.current) clearTimeout(timeoutRef.current)
    }
  }, [])

  const onClick = () => {
    setStatus(onReplace() ? 'replaced' : 'failed')
    if (timeoutRef.current) clearTimeout(timeoutRef.current)
    timeoutRef.current = setTimeout(() => {
      setStatus('idle')
      timeoutRef.current = null
    }, 800)
  }

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50b65a1c83

ℹ️ 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".

Comment on lines +12 to +14
if (tagName === 'TEXTAREA') return true
if (tagName !== 'INPUT') return false
return TEXT_FIELD_INPUT_TYPES.includes((element.type || 'text').toLowerCase())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip read-only text fields

When the selected target is a readonly textarea or text input, this helper still captures it as replaceable, and the later fallback path writes via the native value setter even if execCommand refuses to edit it. That makes the Replace button able to mutate fields the page explicitly marked as non-editable; please exclude readOnly/disabled controls when capturing and re-check before replacing.

Useful? React with 👍 / 👎.

@qodo-code-review

qodo-code-review Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Password text captured ✓ Resolved 🐞 Bug ⛨ Security
Description
captureEditableSelection treats <input type="password"> as a supported text field and stores the
selected substring in the captured descriptor (captured.text), risking credential exposure in
extension memory/state. It also enables the Replace flow to write AI text into password inputs when
invoked.
Code

src/utils/selection-replace.mjs[R7-15]

+const TEXT_FIELD_INPUT_TYPES = ['text', 'search', 'url', 'tel', 'password']
+
+const isTextField = (element) => {
+  if (!element || typeof element.tagName !== 'string') return false
+  const tagName = element.tagName.toUpperCase()
+  if (tagName === 'TEXTAREA') return true
+  if (tagName !== 'INPUT') return false
+  return TEXT_FIELD_INPUT_TYPES.includes((element.type || 'text').toLowerCase())
+}
Evidence
The utility explicitly includes password in the recognized text field types and then captures the
selected substring from activeElement.value into captured.text, which is subsequently passed
through the selection-tools UI path.

src/utils/selection-replace.mjs[7-15]
src/utils/selection-replace.mjs[32-42]
src/content-script/index.jsx[293-332]
src/components/FloatingToolbar/index.jsx[114-118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`captureEditableSelection()` currently supports `input[type=password]` and stores the selected substring in `captured.text`. This is sensitive credential material and should not be captured or made replaceable.
## Issue Context
The supported input types list includes `password`, and the capture descriptor stores the selected value slice.
## Fix Focus Areas
- src/utils/selection-replace.mjs[7-15]
- src/utils/selection-replace.mjs[32-43]
- tests/unit/utils/selection-replace.test.mjs[118-146]
## Implementation notes
- Remove `'password'` from `TEXT_FIELD_INPUT_TYPES` (or explicitly return `false` from `isTextField()` when `element.type === 'password'`).
- Add/adjust unit test ensuring password inputs are ignored (capture returns `null`).
- (Optional hardening) Also ignore elements marked `readOnly`/`disabled` if you want to avoid modifying non-editable controls.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Sensitive selection logged ✓ Resolved 🐞 Bug ⛨ Security
Description
createSelectionTools logs the full capturedSelection object (including selected text and DOM
references) via console.debug, which can expose user-selected content in page devtools logs. This is
a privacy risk whenever debug logs are visible/collected.
Code

src/content-script/index.jsx[R240-248]

+const createSelectionTools = async (toolbarContainerElement, selection, capturedSelection) => {
console.debug(
 '[content] createSelectionTools called with selection:',
 selection,
-    'and container:',
+    'container:',
 toolbarContainerElement,
+    'captured editable selection:',
+    capturedSelection,
)
Evidence
The content script logs capturedSelection as-is, and the captured descriptor contains the selected
substring (text) and DOM references (element/range).

src/content-script/index.jsx[240-248]
src/utils/selection-replace.mjs[39-62]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createSelectionTools` logs `capturedSelection` directly, which includes the selected substring and element/range references. This can leak user-selected content into page console logs.
### Issue Context
The captured selection descriptor includes `text` (the selected content) for both text-fields and contenteditable selections.
### Fix Focus Areas
- src/content-script/index.jsx[240-248]
### Suggested fix
- Change the debug log to avoid printing the full object.
- Option A: remove the `capturedSelection` argument entirely.
- Option B: log only non-sensitive metadata (e.g., `capturedSelection?.kind`, `start/end` offsets, and `text.length`), never the text itself or DOM nodes/ranges.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Selection capture retained ✓ Resolved 🐞 Bug ☼ Reliability
Description
The right-click menu path stores menuCapturedSelection as a module-level variable and never clears
it after handling the menu action, retaining DOM/Range references longer than necessary. On
long-lived pages this can needlessly keep objects alive and is avoidable.
Code

src/content-script/index.jsx[R462-465]

+let menuX, menuY, menuCapturedSelection
let rightClickMenuInitialized = false
async function prepareForRightClickMenu() {
Evidence
The content script captures an editable selection into a persistent variable and later reuses it for
selection-tools actions, but there is no lifecycle cleanup shown for that stored capture.

src/content-script/index.jsx[462-493]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`menuCapturedSelection` is captured on `contextmenu` and later reused when the background script message arrives, but it is never reset afterward.
### Issue Context
This variable can hold references to DOM elements and a cloned Range, which may unnecessarily extend their lifetime.
### Fix Focus Areas
- src/content-script/index.jsx[462-493]
### Suggested fix
- After using `menuCapturedSelection` (e.g., right after assigning `capturedSelection = menuCapturedSelection` or after rendering the `FloatingToolbar` for the CREATE_CHAT message), set `menuCapturedSelection = null`.
- Optionally also clear it when handling non-selection menu tools, to ensure it never lingers beyond the relevant action.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Stale guard misses shifts ✓ Resolved 🐞 Bug ≡ Correctness
Description
For text fields, replacement only checks that value.slice(start,end) still equals the originally
selected text, so some edits elsewhere in the field can go undetected if the same substring remains
at those offsets. In those edge cases, Replace can apply to a stale position instead of failing
safely.
Code

src/utils/selection-replace.mjs[R92-97]

+const replaceInTextField = (captured, text, doc) => {
+  const { element, start, end, text: originalText } = captured
+  if (element.isConnected === false || element.readOnly || element.disabled) return false
+  const value = element.value ?? ''
+  if (value.slice(start, end) !== originalText) return false
+
Evidence
The capture stores only offsets and selected substring, and the replace path uses only substring
equality at those offsets to decide whether the capture is stale.

src/utils/selection-replace.mjs[39-49]
src/utils/selection-replace.mjs[92-97]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The text-field stale guard validates only the selected substring at the captured offsets, not whether the overall field content has changed since capture. This can miss certain content shifts (e.g., repetitive text) and still perform a replacement.
### Issue Context
`captureEditableSelection` stores only `start`, `end`, and `text` for text fields; `replaceInTextField` compares only `value.slice(start,end)` to that captured text.
### Fix Focus Areas
- src/utils/selection-replace.mjs[39-49]
- src/utils/selection-replace.mjs[92-97]
### Suggested fix
- Capture stronger context for text fields and validate it before replacing:
- Simplest/strictest: also store `valueAtCapture` (entire `activeElement.value`) and require `element.value === valueAtCapture` before replacing.
- Less strict: store a small prefix/suffix window around the selection (e.g., N chars before/after) and validate those as well.
- Keep current substring check as an additional guard, but ensure the new check prevents replacing when the field has changed in ways that could shift offsets.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
5. Non-Event input dispatch ✓ Resolved 🐞 Bug ☼ Reliability
Description
dispatchInputEvent uses a plain object when InputEvent is unavailable and passes it to
dispatchEvent, which expects an Event instance. In environments lacking InputEvent, the replace
fallback path can throw or fail to notify pages that rely on input events (e.g., controlled inputs).
Code

src/utils/selection-replace.mjs[R62-69]

+const dispatchInputEvent = (element) => {
+  if (typeof element.dispatchEvent !== 'function') return
+  const InputEventConstructor = globalThis.InputEvent
+  const event = InputEventConstructor
+    ? new InputEventConstructor('input', { bubbles: true, inputType: 'insertText' })
+    : { type: 'input', bubbles: true }
+  element.dispatchEvent(event)
+}
Evidence
The fallback path creates a non-Event object and still calls dispatchEvent(event), and this
function is invoked in the replace fallbacks that rely on dispatching an input event after
programmatic insertion.

src/utils/selection-replace.mjs[62-69]
src/utils/selection-replace.mjs[102-105]
src/utils/selection-replace.mjs[131-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When `globalThis.InputEvent` is missing, `dispatchInputEvent()` constructs a plain object (`{ type: 'input', bubbles: true }`) and passes it to `element.dispatchEvent(...)`. In real DOM, `dispatchEvent` expects an `Event` object.
## Issue Context
This code is used in the manual replacement fallbacks for both text fields and contenteditables.
## Fix Focus Areas
- src/utils/selection-replace.mjs[62-69]
- src/utils/selection-replace.mjs[102-105]
- src/utils/selection-replace.mjs[131-135]
- tests/unit/utils/selection-replace.test.mjs[97-100]
## Implementation notes
- Replace the plain-object fallback with `new Event('input', { bubbles: true })` (or `doc.createEvent('Event')` + `initEvent` if you need an even older fallback).
- Update unit tests to reflect that the dispatched value is an Event-like object (or adjust the test double to accept Event instances).
- Keep the InputEvent path as-is so you still set `inputType: 'insertText'` where supported.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

6. Timer not cleared on unmount ✓ Resolved 🐞 Bug ☼ Reliability
Description
ReplaceButton schedules a timeout that calls setStatus but does not clear it on unmount, so
closing/unmounting the toolbar quickly can trigger setState-after-unmount warnings. This is minor
but avoidable lifecycle risk.
Code

src/components/ReplaceButton/index.jsx[R14-24]

+  const [status, setStatus] = useState('idle')
+  const timeoutRef = useRef(null)
+
+  const onClick = () => {
+    setStatus(onReplace() ? 'replaced' : 'failed')
+    if (timeoutRef.current) clearTimeout(timeoutRef.current)
+    timeoutRef.current = setTimeout(() => {
+      setStatus('idle')
+      timeoutRef.current = null
+    }, 800)
+  }
Evidence
The component stores the timeout handle and schedules a callback that calls setStatus, but there is
no unmount cleanup to clear the timer.

src/components/ReplaceButton/index.jsx[12-24]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReplaceButton` keeps a pending `setTimeout` that may fire after the component unmounts, calling `setStatus`.
## Issue Context
The timer is stored in `timeoutRef` but there is no unmount cleanup.
## Fix Focus Areas
- src/components/ReplaceButton/index.jsx[14-24]
## Implementation notes
- Add `useEffect(() => () => { if (timeoutRef.current) clearTimeout(timeoutRef.current) }, [])`.
- Ensure `timeoutRef.current` is nulled in cleanup for consistency.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/utils/selection-replace.mjs Outdated
Comment thread src/utils/selection-replace.mjs
Comment thread src/components/ReplaceButton/index.jsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/components/ReplaceButton/index.jsx`:
- Around line 15-24: The ReplaceButton timeout is only cleared before scheduling
a new one, but not when the component unmounts, so the delayed callback in
onClick can still call setStatus after unmount. Add a cleanup effect in the
ReplaceButton component that clears timeoutRef.current on unmount, and make sure
it also nulls the ref after clearing so no pending timer survives component
teardown.

In `@src/content-script/index.jsx`:
- Around line 329-331: The mouseup and touchend selection handling in index.jsx
is skipping Firefox text-field selections because it bails out on
window.getSelection().toString() before calling captureEditableSelection().
Update the selection guard in the flows around captureEditableSelection() and
createSelectionTools() to also recognize <input>/<textarea> selections via
document.activeElement.selectionStart/selectionEnd when the document selection
is empty, so toolbar creation still runs for editable-field text selections.

In `@src/utils/selection-replace.mjs`:
- Line 7: Remove password from TEXT_FIELD_INPUT_TYPES in selection-replace.mjs
so captureEditableSelection no longer treats password inputs as text fields;
update the selection capture logic that uses TEXT_FIELD_INPUT_TYPES and
activeElement.value.slice(start, end) to exclude password fields entirely,
ensuring the descriptor text property is never populated from password input
content.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7a54da2e-a370-4599-9c6a-94ec17320d82

📥 Commits

Reviewing files that changed from the base of the PR and between ff3d5f7 and 50b65a1.

📒 Files selected for processing (21)
  • src/_locales/de/main.json
  • src/_locales/en/main.json
  • src/_locales/es/main.json
  • src/_locales/fr/main.json
  • src/_locales/in/main.json
  • src/_locales/it/main.json
  • src/_locales/ja/main.json
  • src/_locales/ko/main.json
  • src/_locales/pt/main.json
  • src/_locales/ru/main.json
  • src/_locales/tr/main.json
  • src/_locales/zh-hans/main.json
  • src/_locales/zh-hant/main.json
  • src/components/ConversationCard/index.jsx
  • src/components/ConversationItem/index.jsx
  • src/components/FloatingToolbar/index.jsx
  • src/components/ReplaceButton/index.jsx
  • src/content-script/index.jsx
  • src/utils/index.mjs
  • src/utils/selection-replace.mjs
  • tests/unit/utils/selection-replace.test.mjs

Comment thread src/components/ReplaceButton/index.jsx
Comment thread src/content-script/index.jsx Outdated
Comment thread src/utils/selection-replace.mjs Outdated
Comment thread src/utils/selection-replace.mjs Outdated

const onClick = () => {
setStatus(onReplace() ? 'replaced' : 'failed')
if (timeoutRef.current) clearTimeout(timeoutRef.current)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: setTimeout created in onClick is not cleared on unmount

If the component unmounts before the 800ms timeout fires, the callback runs and calls setStatus on an unmounted component. Add an effect cleanup (or useEffect) to clear the timer on unmount.

@kilo-code-bot

kilo-code-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

All prior review findings (password capture, readOnly/disabled capture, non-event input dispatch, editing-host resolution, cursor re-application for contenteditable, ReplaceButton unmount cleanup, Firefox text-field selection support, sensitive selection logging, retained menuCapturedSelection, and the stricter whole-field stale guard) have been verified as resolved in the current HEAD (25fbd1e). No new issues were identified in the changed code.

Files Reviewed (20 files)
  • src/utils/selection-replace.mjs - verified resolved
  • src/components/ReplaceButton/index.jsx - verified resolved
  • src/components/ConversationCard/index.jsx - no new issues
  • src/components/ConversationItem/index.jsx - no new issues
  • src/components/FloatingToolbar/index.jsx - no new issues
  • src/content-script/index.jsx - no new issues
  • src/utils/index.mjs - no new issues
  • tests/unit/utils/selection-replace.test.mjs - no new issues
  • src/_locales/{de,en,es,fr,in,it,ja,ko,pt,ru,tr,zh-hans,zh-hant}/main.json - mechanical i18n additions, no new issues
Previous Review Summaries (3 snapshots)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • src/content-script/index.jsx
  • src/utils/selection-replace.mjs
  • tests/unit/utils/selection-replace.test.mjs

Previous review (commit 0fb4fd7)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • src/utils/selection-replace.mjs
  • src/components/ReplaceButton/index.jsx
  • src/content-script/index.jsx
  • tests/unit/utils/selection-replace.test.mjs

Previous review (commit 50b65a1)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 2
Issue Details (click to expand)

SUGGESTION

File Line Issue
src/utils/selection-replace.mjs 67 dispatchInputEvent fallback creates a plain object instead of a valid Event instance
src/components/ReplaceButton/index.jsx 19 setTimeout created in onClick is not cleared on unmount
Files Reviewed (18 files)
  • src/utils/selection-replace.mjs - 1 issue
  • src/components/ReplaceButton/index.jsx - 1 issue
  • src/content-script/index.jsx
  • src/components/ConversationCard/index.jsx
  • src/components/ConversationItem/index.jsx
  • src/components/FloatingToolbar/index.jsx
  • src/utils/index.mjs
  • tests/unit/utils/selection-replace.test.mjs
  • 10 locale files

Reviewed by hy3-20260706:free · Input: 72K · Output: 11.9K · Cached: 176.7K

sebastianelsner and others added 3 commits July 9, 2026 01:09
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The context menu click arrives asynchronously via the background
script, so the editable selection is captured on the contextmenu event
alongside the menu position. Menu tools operating on the whole page
intentionally do not offer replacement.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Exclude password inputs from capture so credential text is never
  stored in the descriptor, and exclude readonly/disabled fields both
  at capture time and again before replacing
- Resolve nested contenteditable selections to the editing host so
  focus() lands on a focusable element before execCommand
- Re-apply the collapsed range to the selection in the contenteditable
  fallback so the cursor ends up after the inserted text
- Dispatch a real Event instance when InputEvent is unavailable
  instead of a plain object
- Clear the ReplaceButton status timeout on unmount
- Fall back to the captured text field selection when
  window.getSelection() is empty, so selection tools also trigger for
  input/textarea selections on Firefox

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sebastianelsner

Copy link
Copy Markdown
Author

Addressed the review feedback in 0fb4fd7:

  • Password inputs excluded (qodo, coderabbitai): password removed from the supported input types — credential text is never captured, and password fields can't be replace targets.
  • readonly/disabled fields excluded (codex): rejected at capture time and re-checked before replacing, so the value-setter fallback can no longer write into controls the page marked non-editable.
  • Editing host resolution (gemini-code-assist): nested contenteditable selections now walk up to the top-most contenteditable ancestor so focus() lands on a focusable element before execCommand.
  • Cursor placement in fallback path (gemini-code-assist): the collapsed range is re-applied to the live selection after inserting the text node.
  • Valid Event fallback (qodo, kilo-code-bot): when InputEvent is unavailable a real Event('input', { bubbles: true }) is dispatched instead of a plain object.
  • ReplaceButton timer cleanup (gemini-code-assist, qodo, coderabbitai, kilo-code-bot): pending status timeout is cleared on unmount.
  • Firefox text-field selections (coderabbitai): the mouseup/touchend flows now fall back to the captured text-field selection when window.getSelection() is empty, so the selection toolbar (and Replace) also works for input/textarea on Firefox.

All covered by new unit tests (24 in selection-replace.test.mjs, 713 total passing); lint and production build clean.

@sebastianelsner sebastianelsner marked this pull request as ready for review July 8, 2026 23:27
Comment thread src/content-script/index.jsx
Comment thread src/content-script/index.jsx
Comment thread src/utils/selection-replace.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0fb4fd7

@sebastianelsner

sebastianelsner commented Jul 9, 2026

Copy link
Copy Markdown
Author

Addressed the second qodo review round in 25fbd1e:

  • Sensitive selection logged: the debug log now prints only the captured selection kind — the selected text and DOM/Range references never reach the page console.
  • Selection capture retained: menuCapturedSelection is cleared as soon as the CREATE_CHAT message consumes it, so element/Range references are not kept alive on long-lived pages.
  • Stale guard misses shifts: the capture now stores the full field value and replacement requires an exact match, so edits that shift the selection offsets while leaving the same substring in place (e.g. repetitive text) fail safely instead of replacing the wrong position. Covered by a new unit test.

(The branch was force-pushed once to drop an accidentally committed local build artifact from history; no code changes.)

714 tests passing, lint and production build clean.

- Log only the captured selection kind instead of the full descriptor
  so selected text and DOM references never end up in page console logs
- Clear the context menu selection capture once the menu action has
  been handled instead of retaining element and range references
- Store the full text field value at capture time and require an exact
  match before replacing, so edits that shift the selection offsets
  while leaving the same substring in place fail safely

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PeterDaveHello

Copy link
Copy Markdown
Member

/gemini review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “Replace Selection” action to the selection-tools answer UI so users can write an AI answer back into the originally selected text inside editable fields (inputs/textareas/contenteditable), avoiding manual copy/paste. This integrates capture + safe replace logic into the content script’s selection-tools flow while keeping non-editable page content non-replaceable.

Changes:

  • Add captureEditableSelection / replaceCapturedSelection utilities with stale/disconnected guards for text fields and contenteditable.
  • Thread captured selection through the selection-tools FloatingToolbar → ConversationCard → ConversationItem and introduce a ReplaceButton with success/failure feedback.
  • Add unit tests for capture/replace behavior and propagate new i18n strings across locales.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/utils/selection-replace.test.mjs New unit tests covering capture + replace for text fields and contenteditable, including stale/disconnected guards.
src/utils/selection-replace.mjs New utility implementing capture + safe replacement (execCommand preferred, fallback for controlled inputs).
src/utils/index.mjs Re-export selection-replace utilities via utils barrel.
src/content-script/index.jsx Capture editable selection before toolbar interaction; pass captured selection into FloatingToolbar for selection-tools flows.
src/components/ReplaceButton/index.jsx New Replace button component with success/failure icon feedback and localized tooltip text.
src/components/FloatingToolbar/index.jsx Wire onReplaceContent into ConversationCard when an editable selection was captured.
src/components/ConversationItem/index.jsx Render ReplaceButton on completed answers when onReplace is provided.
src/components/ConversationCard/index.jsx Provide onReplace callback per completed answer, stripping trailing <hr/> like Copy does.
src/_locales/*/main.json Add localized strings for “Replace Selection” and “Replace Failed”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +33 to +37
<span
title={status === 'failed' ? t('Replace Failed') : t('Replace Selection')}
className={`gpt-util-icon ${className ? className : ''}`}
onClick={onClick}
>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a feature allowing users to replace selected text in editable fields (text inputs, textareas, and contenteditable regions) with the AI-generated response. It includes localized strings, a new ReplaceButton component, selection capture/replacement utilities, and comprehensive unit tests. The review feedback highlights a potential memory leak in the content script due to uncleared context menu selections, potential wrong-document DOM exceptions when interacting with elements inside iframes, and a robustness issue in ReplaceButton where void-returning callbacks incorrectly trigger a failure state.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

menuY = e.clientY
// the menu click arrives asynchronously via the background script, so the
// editable selection has to be captured while it still exists
menuCapturedSelection = captureEditableSelection()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Memory Leak Risk

The global variable menuCapturedSelection is assigned a captured selection when the context menu is opened. However, if the user dismisses the context menu without clicking any of the extension's menu items (e.g., by clicking elsewhere on the page or pressing Escape), CREATE_CHAT is never received, and menuCapturedSelection will continue to hold a reference to the DOM element and the Range object indefinitely.

This prevents the DOM element, its owner document, and associated resources from being garbage collected, leading to a memory leak on long-lived pages.

Suggestion

Clear menuCapturedSelection after a reasonable timeout (e.g., 30 seconds) to prevent memory leaks if the menu is dismissed.

    menuCapturedSelection = captureEditableSelection()
    setTimeout(() => {
      if (menuCapturedSelection) {
        menuCapturedSelection = null
      }
    }, 30000)

Comment on lines +95 to +112
const replaceInTextField = (captured, text, doc) => {
const { element, start, end, text: originalText } = captured
if (element.isConnected === false || element.readOnly || element.disabled) return false
const value = element.value ?? ''
if (value !== captured.fieldValue) return false

if (typeof element.focus === 'function') element.focus()
let replaced = false
if (typeof element.setSelectionRange === 'function' && typeof doc.execCommand === 'function') {
element.setSelectionRange(start, end)
try {
// execCommand keeps the native undo history working where supported
replaced = doc.execCommand('insertText', false, text) && element.value !== value
} catch (error) {
replaced = false
}
if (text === originalText) replaced = true
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Wrong Document Error in Iframes

If the selection is inside an iframe (which is very common for rich text editors), doc (which defaults to globalThis.document, the outer document) will be different from the iframe's document. Calling doc.execCommand on an element inside the iframe will fail or throw a DOMException (Wrong Document Error).

Suggestion

Use element.ownerDocument instead of doc to ensure the command is executed in the correct document context.

const replaceInTextField = (captured, text, doc) => {
  const { element, start, end, text: originalText } = captured
  if (element.isConnected === false || element.readOnly || element.disabled) return false
  const value = element.value ?? ''
  if (value !== captured.fieldValue) return false

  const ownerDoc = element.ownerDocument || doc
  if (typeof element.focus === 'function') element.focus()
  let replaced = false
  if (typeof element.setSelectionRange === 'function' && typeof ownerDoc.execCommand === 'function') {
    element.setSelectionRange(start, end)
    try {
      // execCommand keeps the native undo history working where supported
      replaced = ownerDoc.execCommand('insertText', false, text) && element.value !== value
    } catch (error) {
      replaced = false
    }
    if (text === originalText) replaced = true
  }

Comment on lines +124 to +143
const replaceInContentEditable = (captured, text, doc) => {
const { element, range, text: originalText } = captured
if (element.isConnected === false || !element.isContentEditable) return false
if (range.toString() !== originalText) return false

if (typeof element.focus === 'function') element.focus()
const selection = typeof doc.getSelection === 'function' ? doc.getSelection() : null
if (selection) {
selection.removeAllRanges()
selection.addRange(range)
if (typeof doc.execCommand === 'function') {
try {
if (doc.execCommand('insertText', false, text)) return true
} catch (error) {
// fall through to manual replacement
}
}
}
range.deleteContents()
range.insertNode(doc.createTextNode(text))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Wrong Document Error in Iframes

Similar to text fields, if the contenteditable region is inside an iframe, creating a text node using doc.createTextNode (the outer document) and inserting it into the iframe's range will throw a WRONG_DOCUMENT_ERR DOMException.

Suggestion

Use element.ownerDocument to get the correct document context for creating the text node, retrieving the selection, and executing commands.

const replaceInContentEditable = (captured, text, doc) => {
  const { element, range, text: originalText } = captured
  if (element.isConnected === false || !element.isContentEditable) return false
  if (range.toString() !== originalText) return false

  const ownerDoc = element.ownerDocument || doc
  if (typeof element.focus === 'function') element.focus()
  const selection = typeof ownerDoc.getSelection === 'function' ? ownerDoc.getSelection() : null
  if (selection) {
    selection.removeAllRanges()
    selection.addRange(range)
    if (typeof ownerDoc.execCommand === 'function') {
      try {
        if (ownerDoc.execCommand('insertText', false, text)) return true
      } catch (error) {
        // fall through to manual replacement
      }
    }
  }
  range.deleteContents()
  range.insertNode(ownerDoc.createTextNode(text))

}, [])

const onClick = () => {
setStatus(onReplace() ? 'replaced' : 'failed')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Robustness of Callback Return Value Check

Currently, ReplaceButton assumes that onReplace() strictly returns a truthy value on success. However, in React, many callback functions do not return a value (i.e., they return undefined). If another developer reuses ReplaceButton or ConversationCard with a standard void-returning function, the button will incorrectly show the "Replace Failed" state.

Suggestion

Only treat the result as a failure if it explicitly returns false.

Suggested change
setStatus(onReplace() ? 'replaced' : 'failed')
const success = onReplace()
setStatus(success !== false ? 'replaced' : 'failed')

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Firefox keydown closes toolbar 🐞 Bug ≡ Correctness
Description
The PR adds a Firefox fallback to derive selected text from captured text-field selection, but the
keydown cleanup path still checks only window.getSelection(). In Firefox (where text-field selection
isn’t exposed via window.getSelection), this can incorrectly delete the selection-tools toolbar even
while a text-field selection still exists.
Code

src/content-script/index.jsx[R298-308]

+          const capturedSelection = captureEditableSelection()
+          const selection =
+            window
+              .getSelection()
+              ?.toString()
+              .trim()
+              .replace(/^-+|-+$/g, '') ||
+            // Firefox does not expose text field selections via window.getSelection()
+            (capturedSelection?.kind === 'text-field'
+              ? capturedSelection.text.trim().replace(/^-+|-+$/g, '')
+              : '')
Evidence
The PR acknowledges Firefox text fields don’t surface selection via window.getSelection() and adds a
fallback, but the keydown cleanup still uses window.getSelection() only, which will mis-detect
text-field selections in Firefox and remove the toolbar incorrectly.

src/content-script/index.jsx[295-308]
src/content-script/index.jsx[365-377]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Selection-tools now supports Firefox text-field selections via `captureEditableSelection()`, but the toolbar auto-cleanup on `keydown` still relies on `window.getSelection()`. In Firefox this can evaluate to empty for input/textarea selections, causing premature toolbar deletion.

### Issue Context
The PR explicitly notes Firefox does not expose text-field selections via `window.getSelection()`, and adds a fallback for detecting the selected text on mouseup/touchend. The keydown cleanup should use the same strategy (or directly inspect `selectionStart/selectionEnd`).

### Fix Focus Areas
- src/content-script/index.jsx[365-377]
- src/content-script/index.jsx[298-308]

### Implementation notes
- In the keydown `setTimeout`, decide “selection exists” using either:
 - `window.getSelection()?.toString().trim()` (for contenteditable/static selections), OR
 - for INPUT/TEXTAREA: `e.target.selectionStart < e.target.selectionEnd` (or `captureEditableSelection()` and check `kind === 'text-field'` and non-empty text).
- Only call `deleteToolbar()` when *both* checks indicate there is no selection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Email inputs not supported 🐞 Bug ≡ Correctness
Description
Text-field capture only allows INPUT types in ['text','search','url','tel'], so selections in common
editable inputs like <input type="email"> will never be captured. As a result, Replace will never be
offered for those inputs even though they support selectionStart/selectionEnd.
Code

src/utils/selection-replace.mjs[R7-16]

+const TEXT_FIELD_INPUT_TYPES = ['text', 'search', 'url', 'tel']
+
+const isTextField = (element) => {
+  if (!element || typeof element.tagName !== 'string') return false
+  if (element.readOnly || element.disabled) return false
+  const tagName = element.tagName.toUpperCase()
+  if (tagName === 'TEXTAREA') return true
+  if (tagName !== 'INPUT') return false
+  return TEXT_FIELD_INPUT_TYPES.includes((element.type || 'text').toLowerCase())
+}
Evidence
isTextField() explicitly filters INPUT elements by an allowlist that excludes email, so those
selections cannot be captured and therefore cannot be replaced.

src/utils/selection-replace.mjs[7-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`captureEditableSelection()` currently allowlists only a few INPUT types. This blocks Replace for other text-like inputs (notably `type="email"`).

### Issue Context
`isTextField()` gates whether a selection can be captured/replaced in inputs; anything excluded here makes the Replace feature unavailable for that field type.

### Fix Focus Areas
- src/utils/selection-replace.mjs[7-16]

### Implementation notes
Prefer a capability-based check over a narrow allowlist:
- Allow INPUT elements that expose numeric `selectionStart/selectionEnd` and are not `readOnly/disabled`.
- Keep an explicit denylist for non-text inputs and sensitive types (e.g., `password`, `checkbox`, `radio`, `file`, `submit`, `button`, etc.).
- At minimum, include `email` in the allowed list if you want to stay with allowlisting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. InputEvent creation not guarded 🐞 Bug ☼ Reliability
Description
dispatchInputEvent constructs a new InputEvent without guarding against constructor failures, which
can cause replaceCapturedSelection to return false due to a thrown error after performing the
replacement work. This can lead to incorrect failure feedback even though the content was updated.
Code

src/utils/selection-replace.mjs[R72-80]

+const dispatchInputEvent = (element) => {
+  if (typeof element.dispatchEvent !== 'function') return
+  const event = globalThis.InputEvent
+    ? new globalThis.InputEvent('input', { bubbles: true, inputType: 'insertText' })
+    : globalThis.Event
+    ? new globalThis.Event('input', { bubbles: true })
+    : null
+  if (event) element.dispatchEvent(event)
+}
Evidence
The event is constructed directly without a local guard; any exception thrown here is caught by
replaceCapturedSelection’s outer try/catch, which changes the returned boolean (and UI status) even
if earlier mutation already occurred.

src/utils/selection-replace.mjs[72-80]
src/utils/selection-replace.mjs[163-171]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dispatchInputEvent()` assumes `new InputEvent(...)` is always constructible when `globalThis.InputEvent` exists. If that constructor throws in some environments, the outer replacement call catches and reports failure.

### Issue Context
`replaceCapturedSelection()` wraps the replace helpers in a try/catch and returns `false` on any thrown error, so a failure during event construction/dispatch affects the success signal.

### Fix Focus Areas
- src/utils/selection-replace.mjs[72-80]
- src/utils/selection-replace.mjs[163-171]

### Implementation notes
- Wrap InputEvent creation in a try/catch and fall back to `new Event('input', { bubbles: true })`.
- Consider wrapping `element.dispatchEvent(...)` in try/catch as well to avoid flipping success/failure due to a dispatch error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +298 to +308
const capturedSelection = captureEditableSelection()
const selection =
window
.getSelection()
?.toString()
.trim()
.replace(/^-+|-+$/g, '') ||
// Firefox does not expose text field selections via window.getSelection()
(capturedSelection?.kind === 'text-field'
? capturedSelection.text.trim().replace(/^-+|-+$/g, '')
: '')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Firefox keydown closes toolbar 🐞 Bug ≡ Correctness

The PR adds a Firefox fallback to derive selected text from captured text-field selection, but the
keydown cleanup path still checks only window.getSelection(). In Firefox (where text-field selection
isn’t exposed via window.getSelection), this can incorrectly delete the selection-tools toolbar even
while a text-field selection still exists.
Agent Prompt
### Issue description
Selection-tools now supports Firefox text-field selections via `captureEditableSelection()`, but the toolbar auto-cleanup on `keydown` still relies on `window.getSelection()`. In Firefox this can evaluate to empty for input/textarea selections, causing premature toolbar deletion.

### Issue Context
The PR explicitly notes Firefox does not expose text-field selections via `window.getSelection()`, and adds a fallback for detecting the selected text on mouseup/touchend. The keydown cleanup should use the same strategy (or directly inspect `selectionStart/selectionEnd`).

### Fix Focus Areas
- src/content-script/index.jsx[365-377]
- src/content-script/index.jsx[298-308]

### Implementation notes
- In the keydown `setTimeout`, decide “selection exists” using either:
  - `window.getSelection()?.toString().trim()` (for contenteditable/static selections), OR
  - for INPUT/TEXTAREA: `e.target.selectionStart < e.target.selectionEnd` (or `captureEditableSelection()` and check `kind === 'text-field'` and non-empty text).
- Only call `deleteToolbar()` when *both* checks indicate there is no selection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +7 to +16
const TEXT_FIELD_INPUT_TYPES = ['text', 'search', 'url', 'tel']

const isTextField = (element) => {
if (!element || typeof element.tagName !== 'string') return false
if (element.readOnly || element.disabled) return false
const tagName = element.tagName.toUpperCase()
if (tagName === 'TEXTAREA') return true
if (tagName !== 'INPUT') return false
return TEXT_FIELD_INPUT_TYPES.includes((element.type || 'text').toLowerCase())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Email inputs not supported 🐞 Bug ≡ Correctness

Text-field capture only allows INPUT types in ['text','search','url','tel'], so selections in common
editable inputs like <input type="email"> will never be captured. As a result, Replace will never be
offered for those inputs even though they support selectionStart/selectionEnd.
Agent Prompt
### Issue description
`captureEditableSelection()` currently allowlists only a few INPUT types. This blocks Replace for other text-like inputs (notably `type="email"`).

### Issue Context
`isTextField()` gates whether a selection can be captured/replaced in inputs; anything excluded here makes the Replace feature unavailable for that field type.

### Fix Focus Areas
- src/utils/selection-replace.mjs[7-16]

### Implementation notes
Prefer a capability-based check over a narrow allowlist:
- Allow INPUT elements that expose numeric `selectionStart/selectionEnd` and are not `readOnly/disabled`.
- Keep an explicit denylist for non-text inputs and sensitive types (e.g., `password`, `checkbox`, `radio`, `file`, `submit`, `button`, etc.).
- At minimum, include `email` in the allowed list if you want to stay with allowlisting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +72 to +80
const dispatchInputEvent = (element) => {
if (typeof element.dispatchEvent !== 'function') return
const event = globalThis.InputEvent
? new globalThis.InputEvent('input', { bubbles: true, inputType: 'insertText' })
: globalThis.Event
? new globalThis.Event('input', { bubbles: true })
: null
if (event) element.dispatchEvent(event)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

3. Inputevent creation not guarded 🐞 Bug ☼ Reliability

dispatchInputEvent constructs a new InputEvent without guarding against constructor failures, which
can cause replaceCapturedSelection to return false due to a thrown error after performing the
replacement work. This can lead to incorrect failure feedback even though the content was updated.
Agent Prompt
### Issue description
`dispatchInputEvent()` assumes `new InputEvent(...)` is always constructible when `globalThis.InputEvent` exists. If that constructor throws in some environments, the outer replacement call catches and reports failure.

### Issue Context
`replaceCapturedSelection()` wraps the replace helpers in a try/catch and returns `false` on any thrown error, so a failure during event construction/dispatch affects the success signal.

### Fix Focus Areas
- src/utils/selection-replace.mjs[72-80]
- src/utils/selection-replace.mjs[163-171]

### Implementation notes
- Wrap InputEvent creation in a try/catch and fall back to `new Event('input', { bubbles: true })`.
- Consider wrapping `element.dispatchEvent(...)` in try/catch as well to avoid flipping success/failure due to a dispatch error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants