Skip to content

fix(browser): strengthen contenteditable detection in annotation capture#300

Open
DeryFerd wants to merge 2 commits into
gnoviawan:devfrom
DeryFerd:fix/annotation-sensitive-element-detection
Open

fix(browser): strengthen contenteditable detection in annotation capture#300
DeryFerd wants to merge 2 commits into
gnoviawan:devfrom
DeryFerd:fix/annotation-sensitive-element-detection

Conversation

@DeryFerd

@DeryFerd DeryFerd commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

This fixes a false-positive in the browser annotation capture where contenteditable="false" elements were incorrectly blocked from capture, even though they're explicitly non-editable.

Background

The annotation overlay prevents capturing sensitive elements (form inputs, editable content) to protect user data. The contenteditable detection had a problem: it only checked whether the attribute existed, not what value it had. So contenteditable="false" (which explicitly disables editing) was treated the same as contenteditable="true".

This meant legitimate static content got blocked. Code blocks or formatted text that use contenteditable="false" to prevent accidental edits while keeping text selectable couldn't be captured.

The fix

Updated isSensitiveElement() to check the actual contenteditable value:

  • Block when contenteditable is true, empty string (which means true), or plaintext-only (all editable states)
  • Allow when contenteditable is false (explicitly non-editable)
  • Added element.isContentEditable property check as fallback for DOM-manipulated state

Also added 29 tests in annotation-sensitive-element.test.ts covering all contenteditable values, dynamic type changes, ARIA roles, nested editable elements, and the false-positive scenarios this fixes.

Verification

All 29 new tests pass. TypeScript and Biome checks are clean. The fix maintains security (still blocks actual editable content and form inputs) while fixing the false positives.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced sensitive element detection to more accurately identify editable content areas, improving handling of content-editable field states.
  • Tests

    • Added test coverage for sensitive element detection to ensure accuracy across various input types and editable field variations.

- Improve contenteditable detection to properly handle all values
- contenteditable='false' now correctly allows capture (not sensitive)
- Add support for contenteditable='plaintext-only' detection
- Add isContentEditable property fallback check
- Add comprehensive test suite covering all edge cases

Security improvement: prevents false positives where non-editable
elements were incorrectly blocked from annotation capture.
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@DeryFerd, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 19 minutes and 36 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9947cd59-4888-4f14-b3fa-476c65851977

📥 Commits

Reviewing files that changed from the base of the PR and between f288ed8 and 691781a.

📒 Files selected for processing (2)
  • src-tauri/resources/annotation-overlay.js
  • src/renderer/components/browser/annotation-sensitive-element.test.ts
📝 Walkthrough

Walkthrough

This PR refines the isSensitiveElement detection in the annotation overlay to properly handle contenteditable elements: instead of marking any element with a contenteditable ancestor as sensitive, it now only marks them sensitive when the ancestor is explicitly editable and checks the element's isContentEditable property. A comprehensive Vitest suite validates this behavior across form elements, ARIA roles, contenteditable states, and non-sensitive elements.

Changes

Refined Sensitive Element Detection

Layer / File(s) Summary
Refined contenteditable detection in isSensitiveElement
src-tauri/resources/annotation-overlay.js
isSensitiveElement now distinguishes between explicitly editable contenteditable ancestors ("true", "", "plaintext-only") and non-editable ones ("false"), replacing the prior behavior that marked all elements with a contenteditable ancestor as sensitive.
Comprehensive test suite for sensitive element detection
src/renderer/components/browser/annotation-sensitive-element.test.ts
New Vitest suite loads the overlay script and injects a test hook to verify isSensitiveElement behavior across basic form elements, dynamic type changes, ARIA widget roles, contenteditable variations, non-sensitive elements, and edge cases including datalist and output elements.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 A contenteditable fix so refined,
No more blanket rules unkind,
True, false, or plain—now we see,
Which elements should sensitive be,
Tests guard the logic, crisp and aligned!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: fixing and strengthening contenteditable detection in annotation capture.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 and usage tips.

@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: 1

🤖 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/renderer/components/browser/annotation-sensitive-element.test.ts`:
- Around line 49-103: The test currently re-declares the isSensitiveElement
logic instead of using the production implementation, which risks drifting
tests; modify the production script (annotation-overlay.js) to expose the real
function in test runs by setting window.__termul_test_isSensitiveElement =
isSensitiveElement when window.__termul_test_mode is truthy, then in the test
set window.__termul_test_mode = true before injecting the overlay and remove the
duplicated testExposureScript so the test calls the real isSensitiveElement
implementation.
🪄 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: 5ee36aa0-3bec-4692-9440-dce5c2ed5fa3

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8bb72 and f288ed8.

📒 Files selected for processing (2)
  • src-tauri/resources/annotation-overlay.js
  • src/renderer/components/browser/annotation-sensitive-element.test.ts

Comment thread src/renderer/components/browser/annotation-sensitive-element.test.ts Outdated
@gnoviawan

gnoviawan commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Hey @DeryFerd — triage update 🔁

CI green and mergeable ✅. One open CodeRabbit thread to consider: the test harness in annotation-sensitive-element.test.ts re-declares production logic instead of exercising the real isSensitiveElement (🟠 Major). Worth addressing before merge.

… re-declaring logic

- Modified annotation-overlay.js to expose isSensitiveElement in test mode
- Removed duplicate test logic (60+ lines) from test file
- Test now exercises production code directly, preventing drift
- All 29 tests still pass
@DeryFerd

Copy link
Copy Markdown
Contributor Author

Fixed - Test now uses production code

Removed the test logic duplication issue identified by CodeRabbit.

What changed:

  1. **Modified **: Added test mode support that exposes when is set

    • Only exposes in test mode (production unaffected)
    • Cleanup properly removes the test exposure
  2. Simplified test file: Removed 60+ lines of re-declared logic

    • Test now sets before injection
    • Uses the real from production code
    • No more risk of test/production drift

Benefits:

  • Tests now validate actual production behavior
  • Single source of truth for sensitive element detection
  • Future changes to automatically tested
  • Reduced maintenance burden

All 29 tests still pass ✅

@gnoviawan gnoviawan closed this Jun 18, 2026
@gnoviawan gnoviawan reopened this Jun 18, 2026
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.

2 participants