Allow truncating SVG in AppNodeText

[ptgolden]

Labels in the phenogrid were previously rendered as <text> SVG elements, rather than <AppNodeText> Vue elements which whitelist certain markup. This replaces those text elements with AppNodeText elements.

An issue, though, is that labels were previously truncated based on their bare text length. So the string 'label<b>with bolded</b>' might be truncated to 'label<b' or 'label<b>with bo' rather than 'label with bo' (where "with bo" would be bolded).

This PR fixes that bug by truncating text according to its length as it appears in rendered SVG.

Fixes the bug mentioned in #943 (comment).

[netlify]

✅ Deploy Preview for monarch-app ready!

Name	Link
🔨 Latest commit	9813cf2
🔍 Latest deploy log	https://app.netlify.com/sites/monarch-app/deploys/67ffb8a433fe720007735d60
😎 Deploy Preview	https://deploy-preview-945--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 site configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (042a914) to head (e80acf8).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #945   +/-   ##
=======================================
  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.

[ptgolden]

I am going to add several test cases before continuing

[amc-corey-cox]

This is a great change! This makes it much simpler and we don't have to rely as much on our own internal logic. This both solves the bug and makes the whole thing easier to understand and maintain.

[amc-corey-cox]

Waiting to merge until you make that last small fix.

[sagehrke]

@ptgolden this is going to have conflicts now with all the updates @varun-divya has done. If it still needs to happen, please work with her to get the conflicts resolved. Thanks!

[netlify]

✅ Deploy Preview for monarch-app ready!

Name	Link
🔨 Latest commit	e80acf8
🔍 Latest deploy log	https://app.netlify.com/projects/monarch-app/deploys/698f53f71979690008988f92
😎 Deploy Preview	https://deploy-preview-945--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]

PR Review: Allow truncating SVG in AppNodeText

Overview

This PR refactors the AppNodeText component to use <foreignObject> for SVG rendering and implements CSS-based text truncation instead of JavaScript string manipulation. The change fixes the bug where text with markup tags was incorrectly truncated by raw string length rather than displayed length.

---

✅ Strengths

1. Simplified Architecture

The new approach is significantly cleaner:

• Reduced from ~193 lines to ~86 lines in the main component logic
• Eliminated complex DOM manipulation with Range API and manual text node splitting
• Replaced regex matching and node traversal with simple string replacement patterns

2. Correct Truncation Logic

The core problem is properly solved:

• Old approach: truncated 'labelwith bolded' → 'label<b' (incorrect)
• New approach: uses CSS text-overflow: ellipsis which respects rendered content

3. Excellent Test Coverage

Added comprehensive unit tests (AppNodeText.test.ts) covering:

• Plain text rendering
• All whitelisted tags (bold, italic, superscript, anchor)
• Nested tags
• Non-whitelisted tag escaping
• SVG-specific rendering ( ^{→ conversion)}

4. Better Browser Compatibility

Addressed WebKit-specific ^{positioning issues inside with an elegant workaround (lines 81-102).}

---

🔍 Issues & Concerns

1. Security: Potential XSS Vulnerability ⚠️ CRITICAL

Location: AppNodeText.vue:22

Issue: Using v-html with user-controlled content after manual HTML unescaping (lines 65-77) is dangerous. While the component attempts to whitelist specific tags, the regex-based approach can be bypassed.

Attack vectors:

1. Event handlers: Node <img src=x onerror=alert(1)> - The pattern only checks for specific tags, not attributes
2. JavaScript URLs: <a href="javascript:alert(1)">click</a> - The regex checks for http but doesn't enforce it strictly
3. Nested escaping: Double-encoded payloads might bypass the single-pass replacement

Recommendation:

• Use a battle-tested sanitization library like DOMPurify configured to allow only specific tags
• OR: Create actual Vue components for each tag type and render them properly (no v-html)

2. foreignObject Accessibility Concerns

Location: Lines 12-16

Issue: SVG foreignObject with embedded HTML has known accessibility issues:

• Screen readers may not properly announce text inside foreignObject
• The xmlns attribute on the span is good, but may not be sufficient
• The fixed height of 1.5em could cause clipping with larger font sizes or when users zoom

Recommendation:

• Add role="img" and aria-label attributes when appropriate
• Test with screen readers (NVDA, JAWS, VoiceOver)
• Consider making height configurable or auto-adjusting

3. Regex Pattern Limitations

Location: Lines 46-58

Issues:

• Pattern doesn't handle self-closing tags or malformed HTML
• The .*? in patterns could match across multiple unintended tags if markup is nested incorrectly
• Anchor tag pattern ' href="http[^"]+"' requires http but comment says "starting with http" - should include https

Recommendation:

• Add more defensive parsing
• Update anchor pattern to: ' href="https?://[^"]+"'
• Consider using a proper HTML parser instead of regex

4. Performance: DOM Manipulation in Lifecycle Hooks

Location: Lines 114-122

Issue: buildDOM() runs on both onMounted and onUpdated, which means:

• Every prop change triggers innerHTML replacement and DOM querying
• No memoization or dirty checking
• Could cause performance issues if labels update frequently

Recommendation:

• Use watch() with specific dependencies instead of onUpdated
• Cache processed HTML and only rebuild when text or truncateWidth props change
• Consider using computed properties where possible

5. Missing Edge Cases in Tests

The tests are good but missing some scenarios:

• No test for truncateWidth prop functionality
• No test for the highlight prop
• No test for malformed/mismatched tags
• No test for the anchor tag href validation

6. Code Style: Missing Newline at EOF

Location: AppNodeText.vue:129

The file is missing a newline at the end. This violates POSIX standards and many linters.

7. Removed Functionality Without Deprecation

Location: ThePhenogrid.vue:498-509

The PR removes the truncateBySize() function but:

• No check if it's used elsewhere in the codebase
• The function still exists in util/dom.ts but is now unused in this component
• Consider if this function should be deprecated/removed from the util file as well

---

🎯 Performance Considerations

Positive:

• CSS-based truncation is more performant than JS string measurement
• Removed canvas-based text measurement (truncateBySize)
• Simpler DOM structure

Concerns:

• foreignObject can have rendering performance implications in complex SVGs with many labels
• Re-running buildDOM() on every update without memoization

---

🧪 Test Plan Suggestions

1. Security testing: Attempt XSS payloads through the text prop
2. Visual regression testing: Ensure truncation works correctly across different font sizes, container widths, with and without markup
3. Browser testing: Test foreignObject rendering in Safari/WebKit, Firefox, and Chrome
4. Accessibility testing: Use screen readers to verify SVG text is announced
5. Performance testing: Render phenogrid with many labels and measure frame rate

---

📝 Recommendations Summary

Must Fix (Security):

1. Address XSS vulnerability - either use DOMPurify or avoid v-html entirely

Should Fix (Quality):
2. Improve regex patterns for anchor tags (https support)
3. Add memoization to prevent unnecessary DOM rebuilds
4. Add missing test coverage for truncation and edge cases
5. Fix missing newline at EOF

Nice to Have:
6. Improve accessibility for foreignObject
7. Consider deprecating unused truncateBySize from util/dom.ts

---

✨ Overall Assessment

The PR successfully solves the core problem of incorrect text truncation with markup. The approach is cleaner and more maintainable than the previous complex DOM manipulation. However, the security concern with v-html needs immediate attention before merging.

Recommendation: Request changes to address the XSS vulnerability, then approve with minor fixes.

🤖 Generated with Claude Code

[claude]

Code Review

Thanks for working on this PR! The refactoring from custom DOM manipulation to CSS-based truncation is a solid improvement. Here's my detailed review:

✅ Strengths

1. Simplified Approach: Replacing complex DOM splitting/wrapping logic with CSS truncation (text-overflow: ellipsis) is much cleaner and more maintainable.

2. Proper Text Length Calculation: The new approach fixes the bug where HTML tags were counted in the truncation length (e.g., 'label<b' instead of 'label with bo'). Now the browser handles truncation based on rendered width.

3. Good Test Coverage: The new unit tests in AppNodeText.test.ts cover the key scenarios:

   • Plain text rendering
   • Bold, italic, superscript tags
   • Anchor tags with hrefs
   • SVG-specific rendering
   • Nested tags
   • Non-whitelisted tag escaping

4. WebKit Compatibility Fix: The handling of <sup> tags in SVG foreignObject (lines 81-102) shows awareness of browser quirks. Converting to <span> with manual styling avoids WebKit's relative positioning issues.

5. Testbed Integration: Adding visual examples in PageTestbed.vue is excellent for manual QA and regression testing.

🔍 Potential Issues

1. Security Concern - Potential XSS Vulnerability (Medium Priority)

In AppNodeText.vue:22, the non-SVG path uses v-html="text":

<span
  v-else
  ref="container"
  v-bind="$attrs"
  :class="{ 'highlight-text': highlight }"
  v-html="text"
/>

This renders text as raw HTML before the buildDOM() function runs in onMounted/onUpdated. If text contains user-controlled data with malicious scripts, they could execute before sanitization.

Example attack vector:

text = 'Hello <img src=x onerror="alert(1)">'

This would trigger the onerror handler before buildDOM processes it.

Recommendation: Use {{ text }} (text interpolation) instead of v-html, then let buildDOM selectively unescape only the whitelisted tags. This ensures HTML entities are escaped by default.

Note from test file (line 55-57): There's a FIXME comment acknowledging this was done for highlighting support. Consider alternative approaches like:

• Pre-processing the text prop before passing to the component
• Using a separate prop for safe vs. highlighted rendering

2. foreignObject May Not Work in All Contexts (Low-Medium Priority)

Using <foreignObject> for SVG rendering (line 12) is clever but has limitations:

• Fixed height: height="1.5em" may clip multi-line text or text with descenders
• Export compatibility: When exporting SVG to other formats (PDF, images), foreignObject may not be supported by all renderers
• Accessibility: Screen readers may have issues with foreignObject in some browsers

Recommendation:

• Consider if 1.5em is sufficient for all use cases, especially with superscripts
• Add a comment documenting the foreignObject limitations for future maintainers
• Test the download/export functionality mentioned in ThePhenogrid.vue to ensure SVG exports work

3. Unused Function in dom.ts (Low Priority)

truncateBySize is no longer imported anywhere but still exists in util/dom.ts. This creates dead code.

Recommendation: Remove truncateBySize from util/dom.ts in a follow-up commit, or document if it's intentionally kept for other use cases.

4. Browser Compatibility for CSS Truncation (Low Priority)

The CSS truncation approach (lines 104-111) relies on:

• text-overflow: ellipsis
• white-space: nowrap
• overflow: hidden

This is well-supported but doesn't truncate styled text predictably when markup is involved. For example:

<span style="max-width: 50px">Hello <b>world</b></span>

The truncation will work, but if "world" is bolded and bold text is wider, the ellipsis position might be unexpected.

Recommendation: Test with the actual phenogrid labels to ensure truncation looks good with the expected markup combinations.

🎯 Code Quality

1. Clean Refactoring: The PR successfully removes ~130 lines of complex code while adding comprehensive tests.

2. Good Documentation: Comments explain the WebKit workaround and regex pattern usage.

3. Type Safety: Props are properly typed with TypeScript.

🧪 Testing Recommendations

1. Add test for XSS prevention:

test("Prevents XSS attacks", async () => {
  const props = { text: 'Node <img src=x onerror="alert(1)">' };
  const htmlEl = mount(AppNodeText, { props });
  // Should not render img tag, should escape it
  expect(htmlEl.html()).not.toContain('<img');
});

2. Add test for truncation: The current tests don't verify that truncateWidth actually truncates. Consider adding a test that checks the maxWidth style is applied.

3. Visual regression tests: If you have screenshot testing, add tests for the phenogrid labels before/after.

📊 Performance

The new approach should be more performant:

• Less DOM manipulation
• Browser-native text rendering
• No string splitting/measuring in JavaScript

🔒 Security Summary

Priority fix needed: Change v-html to {{ text }} interpolation to prevent potential XSS. The current implementation has a window of vulnerability before buildDOM runs.

Overall Assessment

This is a good refactoring that simplifies the codebase significantly. The main concern is the XSS vulnerability from using v-html. Once that's addressed, this should be safe to merge.

Recommendation: Address the v-html security issue before merging. The other suggestions are nice-to-haves that could be addressed in follow-up PRs.

---

Great work on the simplification and thorough testing! 🎉

[ptgolden]

1. Security Concern - Potential XSS Vulnerability (Medium Priority)

I'm aware of this concern and it was introduced elsewhere. I'm going to make a new issue about it, but it will take some work to fix that is outside the scope of this PR.

Using for SVG rendering (line 12) is clever but has limitations:

Unfortunately, reliably rendering superscripted text with SVG has inescapable cross-browser inconsistencies. See my commit message in 48457da. This approach should be much better browser-compatibility wise.

I take the accessbility comment, but when this component is rendered in SVG, it's going to be in the case of a data visualization component, which would not be fit for a screen reader anyway.