Repository navigation
fix(export): include diagonal column labels in PNG export - #77
Conversation
The diagonal column labels are HTML drag-handle overlays, not part of the SVG, so the canvas+SVG composite dropped them. The exporter now measures the marked label spans from the DOM (geometry, font, colors via computed style) and paints them between the point canvases and the SVG overlay, matching the DOM stacking order. Canvas has no text wrapping, so labels wrap break-all style to mirror the CSS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR extends PNG export to redraw HTML column labels over the rasterized SVG output. It adds ChangesPNG export label overlay
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant ControlPanel
participant renderMatrixToPngBlob
participant collectLabelPlacements
participant drawLabelPlacements
ControlPanel->>renderMatrixToPngBlob: svgEl, canvasContainer, scale, headerContainer
renderMatrixToPngBlob->>renderMatrixToPngBlob: draw per-cell canvases
renderMatrixToPngBlob->>renderMatrixToPngBlob: rasterize SVG overlay
renderMatrixToPngBlob->>collectLabelPlacements: measure headerContainer labels
collectLabelPlacements-->>renderMatrixToPngBlob: LabelPlacement[]
renderMatrixToPngBlob->>drawLabelPlacements: draw label backgrounds/text
renderMatrixToPngBlob-->>ControlPanel: PNG Blob
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for rendering HTML-based diagonal column labels onto the exported PNG of the scatter-plot matrix. It updates the export utility to measure and collect label placements from the DOM, wrap text using a custom break-all algorithm, and draw them onto the canvas in the correct stacking order. The review feedback highlights two critical issues: first, a potential misalignment and sizing error under browser zoom or CSS transforms due to getBoundingClientRect returning scaled dimensions, which can be resolved by computing and applying a domScale factor; second, a bug where unitless CSS line-height values are parsed as raw pixel values instead of multipliers, leading to overlapping text lines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function collectLabelPlacements( | ||
| headerContainer: HTMLElement, | ||
| origin: { left: number; top: number } | ||
| ): LabelPlacement[] { | ||
| return Array.from( | ||
| headerContainer.querySelectorAll<HTMLElement>('[data-column-label]') | ||
| ).map(el => { | ||
| const rect = el.getBoundingClientRect(); | ||
| const cs = window.getComputedStyle(el); | ||
| const fontSize = parseFloat(cs.fontSize) || 16; | ||
| return { | ||
| text: el.dataset.columnLabel || el.textContent || '', | ||
| left: rect.left - origin.left, | ||
| top: rect.top - origin.top, | ||
| width: rect.width, | ||
| height: rect.height, | ||
| font: `${cs.fontWeight} ${cs.fontSize} ${cs.fontFamily}`, | ||
| color: cs.color, | ||
| background: cs.backgroundColor, | ||
| borderRadius: parseFloat(cs.borderRadius) || 0, | ||
| paddingX: parseFloat(cs.paddingLeft) || 0, | ||
| lineHeight: parseFloat(cs.lineHeight) || fontSize * 1.2, | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Issue: DOM Scale / Zoom Misalignment & Unitless Line-Height
- DOM Scale / Zoom Misalignment (High Severity): Viewport coordinates and dimensions returned by
getBoundingClientRect()are scaled by browser zoom and any active CSS transforms (such as the fluid zoom gesture scale). However, the export canvas uses unscaled dimensions. This mismatch causes the exported labels to be misaligned and incorrectly sized when scaling is present. - Unitless Line-Height (Medium Severity): If
getComputedStylereturns a unitless line-height (like1.5or1.2),parseFloatwill return the multiplier directly, leading to overlapping text lines in the canvas. We should guard against this by checking if the parsed value is less than the font size and treating it as a multiplier.
Solution
We can accept an optional domScale factor (defaulting to 1) and divide the viewport-relative coordinates (left, top, width, height) by it to bring them back into the unscaled CSS pixel space. We can also robustly handle unitless line-heights by multiplying them by the font size if they are less than the font size.
export function collectLabelPlacements(
headerContainer: HTMLElement,
origin: { left: number; top: number },
domScale = 1
): LabelPlacement[] {
return Array.from(
headerContainer.querySelectorAll<HTMLElement>('[data-column-label]')
).map(el => {
const rect = el.getBoundingClientRect();
const cs = window.getComputedStyle(el);
const fontSize = parseFloat(cs.fontSize) || 16;
const parsedLineHeight = parseFloat(cs.lineHeight);
const lineHeight = !isNaN(parsedLineHeight)
? (parsedLineHeight < fontSize ? parsedLineHeight * fontSize : parsedLineHeight)
: fontSize * 1.2;
return {
text: el.dataset.columnLabel || el.textContent || '',
left: (rect.left - origin.left) / domScale,
top: (rect.top - origin.top) / domScale,
width: rect.width / domScale,
height: rect.height / domScale,
font: `${cs.fontWeight} ${cs.fontSize} ${cs.fontFamily}`,
color: cs.color,
background: cs.backgroundColor,
borderRadius: parseFloat(cs.borderRadius) || 0,
paddingX: parseFloat(cs.paddingLeft) || 0,
lineHeight,
};
});
}There was a problem hiding this comment.
Fixed in 0db7165 — collectLabelPlacements now takes a domScale (derived from the SVG's rect/attribute width ratio) and unitless line-heights are treated as multipliers.
| if (headerContainer) { | ||
| const origin = svgEl.getBoundingClientRect(); | ||
| drawLabelPlacements(ctx, collectLabelPlacements(headerContainer, origin)); | ||
| } |
There was a problem hiding this comment.
Issue: Missing Scale Factor in Label Placement Collection
To support correct positioning and sizing under browser zoom or CSS transforms, we should compute the domScale factor of the SVG element and pass it to collectLabelPlacements.
| if (headerContainer) { | |
| const origin = svgEl.getBoundingClientRect(); | |
| drawLabelPlacements(ctx, collectLabelPlacements(headerContainer, origin)); | |
| } | |
| if (headerContainer) { | |
| const origin = svgEl.getBoundingClientRect(); | |
| const domScale = origin.width / (Number(svgEl.getAttribute('width')) || svgEl.clientWidth || 1) || 1; | |
| drawLabelPlacements(ctx, collectLabelPlacements(headerContainer, origin, domScale)); | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85bdde25e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| drawLabelPlacements(ctx, collectLabelPlacements(headerContainer, origin)); | ||
| } | ||
| const svgImage = await loadImage(svgToDataUrl(svgEl)); | ||
| ctx.drawImage(svgImage, 0, 0, width, height); |
There was a problem hiding this comment.
When exporting a matrix with diagonal axes, this paints the HTML labels before rasterizing the SVG. In the live view the header overlay is absolutely positioned above the SVG, while the SVG still draws diagonal tick lines/text in components/ScatterPlotMatrix.tsx:1227-1240; with this order the exported PNG puts those ticks on top of the new labels and can partially obscure them. Move the label drawing after ctx.drawImage(svgImage, ...) or otherwise mirror the live stacking order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the header divs are positioned elements so they paint above the static SVG. Labels are now drawn after the SVG overlay (0db7165), re-verified in a real browser.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/exportPng.test.ts (1)
91-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback
rect()path indrawLabelPlacementsis never exercised.
ctx.roundRectis mocked as avi.fn()here, sotypeof ctx.roundRect === 'function'is always true in this test. Thectx.rect(...)fallback branch (used whenroundRectis unavailable, e.g. older engines) indrawLabelPlacementsatsrc/utils/exportPng.tshas no test coverage.Suggested additional test case
+ it('falls back to rect() when roundRect is unavailable', () => { + const calls: string[] = []; + const ctx = { + save: vi.fn(), + restore: vi.fn(), + beginPath: vi.fn(), + rect: vi.fn(() => calls.push('rect')), + fill: vi.fn(() => calls.push('fill')), + fillText: vi.fn((text: string) => calls.push(`text:${text}`)), + measureText: (t: string) => ({ width: t.length * 10 }), + font: '', + fillStyle: '', + textAlign: '', + textBaseline: '', + } as unknown as CanvasRenderingContext2D; + + drawLabelPlacements(ctx, [placement]); + + expect(calls[0]).toBe('rect'); + });🤖 Prompt for 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. In `@src/test/exportPng.test.ts` around lines 91 - 127, The `drawLabelPlacements` test only covers the `roundRect` branch because `ctx.roundRect` is always mocked, so the `ctx.rect()` fallback in `drawLabelPlacements` is untested. Add a separate test in `exportPng.test.ts` that uses the same `drawLabelPlacements` setup but explicitly makes `roundRect` unavailable (for example, omit it or set it to undefined on the mocked `CanvasRenderingContext2D`) so the fallback path runs and `rect()` is called before `fill()`.
🤖 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.
Nitpick comments:
In `@src/test/exportPng.test.ts`:
- Around line 91-127: The `drawLabelPlacements` test only covers the `roundRect`
branch because `ctx.roundRect` is always mocked, so the `ctx.rect()` fallback in
`drawLabelPlacements` is untested. Add a separate test in `exportPng.test.ts`
that uses the same `drawLabelPlacements` setup but explicitly makes `roundRect`
unavailable (for example, omit it or set it to undefined on the mocked
`CanvasRenderingContext2D`) so the fallback path runs and `rect()` is called
before `fill()`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c08ade53-7f74-4d4e-a072-ea6ca8fcd619
📒 Files selected for processing (4)
components/ControlPanel.tsxcomponents/ScatterPlotMatrix.tsxsrc/test/exportPng.test.tssrc/utils/exportPng.ts
There was a problem hiding this comment.
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 @.claude/settings.local.json.tmp.1205318.49b90770fdf6:
- Around line 1-34: Remove the generated Claude permissions file from the change
set so it is not committed, since
.claude/settings.local.json.tmp.1205318.49b90770fdf6 is a local-only settings
artifact. Keep this file untracked and update the repo’s ignore rules to cover
.claude/settings.local.json* so future generated permission files are not picked
up; the fix should be applied around the local Claude settings artifact, not any
application code.
🪄 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 Plus
Run ID: 06970135-7b47-488e-a41f-d79115d784ce
📒 Files selected for processing (3)
.claude/settings.local.json.tmp.1205318.49b90770fdf6src/test/exportPng.test.tssrc/utils/exportPng.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/exportPng.test.ts
- src/utils/exportPng.ts
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(python3 -)", | ||
| "Bash(npx tsc *)", | ||
| "Bash(nnpm ls *)", | ||
| "Bash(npm install *)", | ||
| "Bash(node_modules/.bin/tsc --noEmit)", | ||
| "Bash(npm run *)", | ||
| "Bash(npx vitest *)", | ||
| "Bash(git checkout *)", | ||
| "Bash(kill %1)", | ||
| "Bash(pkill -f \"vite\")", | ||
| "Bash(curl -s -o /dev/null -w \"%{http_code}\" --max-time 2 http://localhost:3000)", | ||
| "Bash(git add *)", | ||
| "Bash(git commit -m ' *)", | ||
| "Bash(git push *)", | ||
| "Bash(gh pr create --title 'Category legend: counts, size-ranked z-order, click-to-hide' --body ' *)", | ||
| "Bash(gh pr *)", | ||
| "Bash(gh api *)", | ||
| "Bash(curl -s -o /dev/null -w \"%{http_code}\\\\n\" http://localhost:3000)", | ||
| "Bash(pkill -f vite)", | ||
| "Bash(pkill -f \"node_modules/.bin/vite\")", | ||
| "Bash(kill 1075450)", | ||
| "Bash(curl -s -o /dev/null --max-time 2 http://localhost:3000)", | ||
| "Bash(git pull *)", | ||
| "Bash(git branch *)", | ||
| "Bash(git ls-remote *)", | ||
| "Bash(gh run *)", | ||
| "Bash(git commit -m 'fix\\(export\\): include diagonal column labels in PNG export *)", | ||
| "Bash(git commit -m 'fix: address bot review feedback on PNG label export *)" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the local Claude permissions file from the PR.
This generated-looking .tmp settings file grants broad local agent permissions, including npm install, git push, gh api, and process-kill commands. It should stay untracked; remove it and consider ignoring .claude/settings.local.json*.
🤖 Prompt for 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.
In @.claude/settings.local.json.tmp.1205318.49b90770fdf6 around lines 1 - 34,
Remove the generated Claude permissions file from the change set so it is not
committed, since .claude/settings.local.json.tmp.1205318.49b90770fdf6 is a
local-only settings artifact. Keep this file untracked and update the repo’s
ignore rules to cover .claude/settings.local.json* so future generated
permission files are not picked up; the fix should be applied around the local
Claude settings artifact, not any application code.
There was a problem hiding this comment.
@dbolser, thanks for that — that sounds like it addresses the separate roundRect/rect() fallback test comment on src/utils/exportPng.ts.
This particular thread is about the generated .claude/settings.local.json.tmp.1205318.49b90770fdf6 file, which grants broad local permissions (npm install, git push, gh api, process-kill commands, etc.) and looks like a local dev artifact that shouldn't be committed. Could you confirm whether you still plan to remove it from this PR and add .claude/settings.local.json* to .gitignore so it doesn't get picked up again in future commits?
- Draw labels after the SVG overlay: the header divs are positioned elements, so they paint above the static SVG in the live view (codex). - Normalize label rects by the SVG's rect/attribute width ratio so an active CSS transform can't misplace the exported labels (gemini). - Guard against unitless computed line-height (gemini). - Cover the rect() fallback branch in tests (coderabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7a80b67 to
0db7165
Compare
Problem
PNG export was missing the diagonal column labels (e.g. PC1, PC2) that are visible in the browser.
Root cause
The diagonal labels aren't part of the SVG — they're HTML
<div>drag-handles absolutely positioned over the plot. The exporter composited only the point canvases and the SVG overlay, so the labels were silently dropped.Fix
data-column-labeland give the header overlay an id so the exporter can find them.collectLabelPlacements()measures each label's geometry and computed style (font, color, background, border radius) from the live DOM.drawLabelPlacements()paints the rounded background and text onto the export canvas between the point canvases and the SVG, matching the DOM stacking order.wrapTextBreakAll), mirroring the label's CSSbreak-all.Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests