Revert "Allow truncating SVG in AppNodeText"

[ptgolden]

Reverts #945

There were some lingering bugs in SVG rendering that need to be looked at before merge.

[netlify]

✅ Deploy Preview for monarch-app ready!

Name	Link
🔨 Latest commit	17bce6f
🔍 Latest deploy log	https://app.netlify.com/projects/monarch-app/deploys/6993451f420660000819e3ea
😎 Deploy Preview	https://deploy-preview-1259--monarch-app.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[claude]

Pull Request Review

This PR reverts #945 due to lingering SVG rendering bugs. I've reviewed the changes and have the following feedback:

Code Quality ✅

Positive aspects:

• The revert cleanly removes the complex DOM manipulation logic from AppNodeText.vue (lines 157-231)
• Simplifies the component back to a more maintainable state
• Removes the truncateWidth prop that was causing issues
• The comment explaining why simpler approaches don't work remains valuable documentation

Potential Issues ⚠️

1. Loss of Functionality
The revert removes the ability to properly truncate SVG text with markup. The phenogrid now falls back to truncateBySize() which:

• Works on plain text only (frontend/src/components/ThePhenogrid.vue:494-500)
• May incorrectly truncate labels containing markup like <sup>, <i>, <b>, or <a> tags
• Could cut tags mid-token (e.g., 'label<b' instead of 'label')

2. Test Coverage Removed
The deletion of frontend/unit/AppNodeText.test.ts removes all test coverage for:

• Plain text rendering
• Bold, italic, superscript rendering
• Anchor tag with href rendering
• SVG-specific rendering (foreignObject → tspan conversion)
• Tag escaping behavior
• Nested tag rendering

Recommendation: When re-implementing the feature, restore these tests to prevent regression.

3. Inconsistent Rendering in AppNodeText
The current code at AppNodeText.vue:34 uses v-html="text" for HTML rendering but only {{ text }} for SVG. This means:

• HTML version will render markup unsafely (potential XSS if text comes from untrusted sources)
• SVG version renders everything as plain text
• The buildDOM() function in the component is never executed for SVG since it only gets {{ text }}

Critical: The buildDOM() function (lines 157-231) won't work because the SVG template uses interpolation instead of a container element with actual content.

Security Concerns 🔒

XSS Vulnerability in HTML Mode:
Line 34 uses v-html="text" without sanitization. If text contains user-controlled content, this could allow XSS attacks. The comment mentions DOMPurify was considered but rejected - however, the current implementation is unsafe.

Recommendation: Either:

1. Use DOMPurify with a whitelist of allowed tags (<sup>, <i>, <b>, <a>)
2. Implement proper escaping before selective unescaping (as mentioned in comments)
3. Document that this component should ONLY receive trusted content

Performance Considerations 💡

The buildDOM() function has good performance characteristics:

• Uses DOM manipulation instead of regex replacement
• Efficiently uses Range.surroundContents()
• However, it runs on both onMounted and onUpdated which could be expensive for frequently updating components

Question: Is there a specific SVG rendering bug mentioned in the PR description? The code review doesn't reveal obvious bugs, but there may be browser-specific issues.

Architecture Notes 📐

The revert is appropriate if there are unresolved bugs, but consider:

1. Keeping the truncateBySize() function - it's useful for plain text
2. Filing issues for the specific SVG bugs encountered
3. Adding integration tests for the phenogrid with truncated labels before re-implementing

Recommendations

Before merging:

• Document the specific SVG rendering bugs that prompted this revert
• File follow-up issues to track re-implementation with proper fix
• Consider adding a comment to ThePhenogrid.vue noting that truncation doesn't work correctly with markup

For future implementation:

• Restore test coverage from the removed test file
• Address the XSS concern in HTML mode
• Add browser-specific testing for SVG rendering
• Consider using a library like svg.js or d3 for complex SVG text manipulation

---

Overall, the revert is reasonable given reported bugs, but the loss of functionality and test coverage should be tracked for future restoration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (e32ef46) to head (17bce6f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1259   +/-   ##
=======================================
  Coverage   73.51%   73.51%           
=======================================
  Files          95       95           
  Lines        3417     3417           
=======================================
  Hits         2512     2512           
  Misses        905      905           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.