Guard against invalid removeChild

[RobbieTheWagner]

Summary by CodeRabbit

• Bug Fixes
  • Improved robustness of DOM removal and cleanup so destroying components no longer errors when elements or their parents have already been removed.
• Tests
  • Added tests ensuring destroy/cleanup operations handle removed elements gracefully without throwing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Added defensive guards and error handling around DOM removals: optional chaining for removeChild calls, try/catch around marker removal, a corrected forEach callback signature, and a guarded zeroElement removal; tests added to validate destroy behavior when nodes are already removed.

Changes

Cohort / File(s)	Summary
Tether defensive removals src/js/tether.js	Replaced direct removeChild calls with optional chaining checks, wrapped marker removals in try { ... } catch { ... }, and corrected a forEach callback signature. Changes avoid errors when elements or parents are absent.
Bounds utility guard src/js/utils/bounds.js	removeUtilElements now verifies zeroElement is a child of the provided body (zeroElement && zeroElement?.parentNode === body) before calling removeChild, then clears the cache.
Test coverage test/unit/tether.spec.js	Added "defensive DOM removal" suite with tests ensuring Tether.destroy does not throw when target, element, or both have been removed from the DOM.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Small, focused defensive changes but touch core cleanup paths.
• Areas needing attention:
  • Verify each optional chaining location in src/js/tether.js covers all null/undefined scenarios.
  • Ensure the try/catch around marker removal doesn't hide unrelated errors — consider logging or rethrowing unexpected exceptions if needed.
  • Confirm zeroElement?.parentNode === body correctly handles cross-document or detached-node cases and doesn't leak nodes.

Poem

🐰 I hopped through code with careful paws,
Checking parents and guarding claws.
Optional chains snug and catch blocks bright,
Now cleanup hushes without a fright.
A tiny rabbit cheers the quiet night.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Guard against invalid removeChild' directly and specifically describes the main change: adding defensive guards and error handling to prevent removeChild operations on already-removed or detached DOM elements.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch defensive-remove-child

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/js/utils/bounds.js (1)

145-150: Defensive zeroElement removal looks correct; consider when to clear the reference

The new zeroElement && zeroElement?.parentNode === body guard is a good defensive fix and should prevent DOM exceptions when the cached node has been detached or belongs to a different body.

One minor refinement: in the case where zeroElement exists but is not a child of body, you still null out the reference without removing the node, which can leave a stray helper <div> in the DOM. Not a big deal, but you could optionally only clear zeroElement after a successful removal, or fall back to removing it from its actual parent if you want stronger cleanup guarantees.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dc7a68 and ec7bcbb.

📒 Files selected for processing (3)

• src/js/tether.js (3 hunks)
• src/js/utils/bounds.js (1 hunks)
• test/unit/tether.spec.js (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/unit/tether.spec.js (1)

src/js/tether.js (1)

• Tether (775-775)

🪛 GitHub Actions: CI Build

test/unit/tether.spec.js

[error] 178-178: 'originalTarget' is assigned a value but never used no-unused-vars

src/js/utils/bounds.js

[warning] 46-46: Function 'getScrollHandleBounds' has a complexity of 9. Maximum allowed is 6 complexity

src/js/tether.js

[warning] 140-140: Method 'setOptions' has a complexity of 8. Maximum allowed is 6 complexity

---

[warning] 296-296: Method 'updateAttachClasses' has a complexity of 10. Maximum allowed is 6 complexity

---

[warning] 312-312: File has too many lines (672). Maximum allowed is 250 max-lines

---

[warning] 367-367: Method 'position' has a complexity of 22. Maximum allowed is 6 complexity

---

[warning] 570-570: Method 'move' has a complexity of 35. Maximum allowed is 6 complexity

---

[warning] 602-602: Arrow function has a complexity of 10. Maximum allowed is 6 complexity

---

[warning] 811-811: Method 'position' has a complexity of 8. Maximum allowed is 6 complexity

🪛 GitHub Check: Tests

test/unit/tether.spec.js

[failure] 178-178:
'originalTarget' is assigned a value but never used

🔇 Additional comments (3)

src/js/tether.js (2)

680-724: Safe reparenting in move() matches the defensive-removal goal

Switching the raw parentNode.removeChild(this.element) calls to this.element?.parentNode?.removeChild(this.element) in the deferred reparenting logic reduces the chance of DOM exceptions when the element or its parent is removed between scheduling and execution. The subsequent appendChild calls preserve the original behavior for the common case where this.element is still a live node.

This is a good, minimal change aligned with the PR’s objective.

---

796-808: Marker cleanup is now robust to missing parents and prior removals

The updated destroy() handler for markers:

• Guards against this.markers or this.markers[type] being absent.
• Uses el?.parentNode?.removeChild(el) inside a try/catch to tolerate parents that have already been removed or other DOM quirks.

Given this is destroy-time cleanup, swallowing these errors is reasonable and prevents teardown from throwing in dynamic DOM environments.

test/unit/tether.spec.js (1)

163-260: New defensive-destroy tests exercise the right edge cases

The added describe('defensive DOM removal', ...) suite appropriately covers:

• Target removed before destroy().
• Element removed before destroy().
• Both removed before destroy().

Disabling the tether before manual removals and then recreating/resetting element/target so the shared afterEach can run cleanly keeps these tests robust without overcomplicating the setup/teardown.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/unit/tether.spec.js (1)

182-204: Optionally align target removal with the guarded pattern used for element

Here you call document.body.removeChild(target) directly, whereas in the later tests you use a parentNode guard before removing nodes. For resilience if target’s parent ever stops being document.body, you could match that pattern:

-      // Remove target from DOM before destroying tether
-      document.body.removeChild(target);
+      // Remove target from DOM before destroying tether
+      if (target.parentNode) {
+        target.parentNode.removeChild(target);
+      }

Not critical, but it keeps your tests robust if Tether’s DOM placement strategy evolves.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec7bcbb and 4c8f312.

📒 Files selected for processing (1)

• test/unit/tether.spec.js (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/unit/tether.spec.js (1)

src/js/tether.js (1)

• Tether (775-775)

🔇 Additional comments (1)

test/unit/tether.spec.js (1)

163-280: New defensive DOM removal tests correctly exercise destroy() behavior and manage shared DOM state

The three tests under describe('defensive DOM removal') do a good job of:

• Verifying destroy() doesn’t throw when the target, element, or both are removed from the DOM first.
• Explicitly disabling before teardown to avoid incidental positioning work during the scenario you want to test.
• Re-establishing fresh element/target instances so the shared afterEach can run without hitting removeChild on detached nodes.

The interplay with the outer beforeEach/afterEach looks sound and shouldn’t leak DOM state across tests.