Claude finished @Yeraze's task —— View job

PR Review: feat(nodes): add CSV/HTML export of the node list

• Read changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Performance considerations
• Security concerns
• Test coverage assessment

Overall this is a clean, well-structured implementation. The separation of pure transform logic (nodeExport.ts) from the DOM side-effect (downloadTextFile) is a solid design choice, and the tests cover the important edge cases. A few findings below, ranging from a real bug to minor polish suggestions.

Bugs

1. Memory leak in downloadTextFile on error (src/utils/nodeExport.ts:207-217)

export function downloadTextFile(filename: string, content: string, mimeType: string): void {
  const blob = new Blob([content], { type: mimeType });
  const url = URL.createObjectURL(blob);
  const a = document.createElement('a');
  a.href = url;
  a.download = filename;
  document.body.appendChild(a);
  a.click();
  document.body.removeChild(a);
  URL.revokeObjectURL(url);   // ← only reached if nothing above throws
}

If any call between createObjectURL and revokeObjectURL throws, the object URL is never revoked and leaks until the page is unloaded. A try/finally would guard it:

const url = URL.createObjectURL(blob);
try {
  const a = document.createElement('a');
  a.href = url;
  a.download = filename;
  document.body.appendChild(a);
  a.click();
  document.body.removeChild(a);
} finally {
  URL.revokeObjectURL(url);
}

2. RFC 4180 CSV — missing final CRLF (src/utils/nodeExport.ts:131-138)

RFC 4180 §2.4 says "Each record is located on a separate line, delimited by a line break (CRLF)" and §2.5 "The last record in the file may or may not have an ending line break." Most consumers handle the absent terminator fine, but some strict parsers don't. Consider appending \r\n after joining, or note it's intentionally omitted.

3. nodesToCsv test relies on header having no quoted fields (src/utils/nodeExport.test.ts:103-106)

it('emits a header row matching the column labels', () => {
  const csv = nodesToCsv([]);
  expect(csv).toBe(NODE_EXPORT_COLUMNS.map((c) => c.label).join(','));

This test would silently break if a label ever gained a comma or quote (e.g. "SNR (dB)" already contains no comma, but "Battery (%)" doesn't either — however the assertion pattern won't catch it). Minor, but worth hardening to use escapeCsv on each label.

Performance

4. Export menu re-register on every showExportMenu toggle (src/components/NodesTab.tsx:1303-1319)

useEffect(() => {
  if (!showExportMenu) return;
  document.addEventListener('mousedown', onPointerDown);
  document.addEventListener('keydown', onKey);
  return () => { ... };
}, [showExportMenu]);

This is the standard pattern for "attach only while open" and is fine as-is. No action needed; called out only because it's a common area of confusion.

5. displayedNodes memo result used as alias sortedNodes = displayedNodes (src/components/NodesTab.tsx:1622)

const sortedNodes = displayedNodes;

This is purely a cosmetic rename inside the render block. It's harmless but adds a little noise; could just use displayedNodes directly, or add a comment that it's intentional backward-compat for the existing render code.

Security

6. HTML export: title option is escaped, but subtitle uses escapeHtml only on the joined string (src/utils/nodeExport.ts:158-163)

const subtitle = [
  `${rows.length} node${rows.length === 1 ? '' : 's'}`,
  opts.generatedAt ? `generated ${opts.generatedAt}` : null,
]
  .filter(Boolean)
  .join(' • ');

opts.generatedAt is then passed through escapeHtml(subtitle) at line 192, so this is safe — the escaping happens after the join. Just noting it because the escaping happens at the interpolation site (line 192) rather than at construction, which could be missed in a future edit. No action required, but worth being aware of.

7. escapeHtml doesn't escape single quotes (src/utils/nodeExport.ts:141-147)

function escapeHtml(value: string): string {
  return value
    .replace(/&/g, '&')
    .replace(/</g, '&lt;')
    .replace(/>/g, '&gt;')
    .replace(/"/g, '&quot;');
}

Single quotes (') are not escaped. In this specific usage the escaped values are only placed inside <td>...</td> content and <title>...</title> — never inside single-quoted HTML attributes — so XSS isn't a concern here. But if escapeHtml is copied elsewhere or used in an attribute context, the omission could become a problem. Adding ' for ' would make it more defensive.

Minor code quality / UX

8. Export button uses ⬇ (U+2B07 DOWNWARDS BLACK ARROW) (NodesTab.tsx:1579)

⬇ is semantically a "down arrow" but the tooltip says "Export node list." The file-download idiom in the rest of the UI uses different icons (e.g. audit log uses a CSV label). Minor UX nit — no action needed if the team is happy with it.

9. aria-expanded on the trigger button could improve screen-reader UX

aria-expanded={showExportMenu} is present — good. However the menu div doesn't have an id and the button doesn't have aria-controls, so screen readers can't navigate between them. Accessibility is usually out of scope for a feature flag like this, but worth noting for a follow-up.

Test coverage

The test file is thorough for the pure transform functions. One gap:

10. No test for nodesToCsv with empty rows returning only a header (the test at line 103 covers this indirectly, but it doesn't assert \r\n is absent at the end — i.e. no trailing newline test)

11. downloadTextFile is untested — this is intentional per the module doc ("isolated DOM side-effect"), and that's fine given the complexity of mocking the DOM. The try/finally fix in point 1 would be the main thing to add here.

Summary

The memory leak in downloadTextFile (#1) is the only item I'd flag as worth fixing before merge. The rest are low-severity or informational.