Skip to content

Commit f2f75da

Browse files
fix: layout and export edge cases from code review
- SVG: Include unbound arrows in bounds calculation (fixes clipping) - SVG: Filter self-referencing arrows in graph builder - PDF: Log Playwright fallback errors instead of silent catch - PDF: Use format parameter in pdf-lib fallback (was hardcoded to Letter) - Force layout: Add forceX/forceY centering for disconnected nodes - Tests: Add edge case tests for empty graphs, missing dimensions, self-referencing arrows, XML escaping, disconnected graphs
1 parent bfefc14 commit f2f75da

5 files changed

Lines changed: 102 additions & 5 deletions

File tree

src/export/pdf.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,17 @@ async function convertSvgToPdfWithPlaywright(
6666

6767
async function convertSvgToPdfWithPdfLib(
6868
svg: string,
69-
_options: PdfExportOptions
69+
options: PdfExportOptions
7070
): Promise<Buffer> {
7171
const pdfLib = await import('pdf-lib')
7272
const { PDFDocument } = pdfLib
7373

7474
const pdfDoc = await PDFDocument.create()
75-
const page = pdfDoc.addPage([612, 792])
75+
const paper = PAPER_SIZES[options.format ?? 'a4']
76+
const isLandscape = options.landscape ?? false
77+
const pageWidth = isLandscape ? paper.height : paper.width
78+
const pageHeight = isLandscape ? paper.width : paper.height
79+
const page = pdfDoc.addPage([pageWidth, pageHeight])
7680

7781
page.drawText('SVG Export (PDF-lib fallback)', {
7882
x: 50,
@@ -105,7 +109,8 @@ export async function exportPdf(
105109
let buffer: Buffer
106110
try {
107111
buffer = await convertSvgToPdfWithPlaywright(svg, options)
108-
} catch {
112+
} catch (err) {
113+
console.warn('Playwright PDF export failed, falling back to pdf-lib:', err)
109114
buffer = await convertSvgToPdfWithPdfLib(svg, options)
110115
}
111116

src/export/svg.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,14 @@ function computeBounds(elements: CanvasElement[]): SvgBounds {
4545
let maxY = -Infinity
4646

4747
for (const el of elements) {
48-
if (el.type === 'arrow' || el.type === 'line') continue
48+
if (el.type === 'arrow' || el.type === 'line') {
49+
if (el.startElementId || el.endElementId) continue
50+
minX = Math.min(minX, el.x)
51+
minY = Math.min(minY, el.y)
52+
maxX = Math.max(maxX, el.x + 1)
53+
maxY = Math.max(maxY, el.y + 1)
54+
continue
55+
}
4956

5057
minX = Math.min(minX, el.x)
5158
minY = Math.min(minY, el.y)

src/layout/force.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { forceSimulation, forceLink, forceManyBody, forceCenter, forceCollide } from 'd3-force'
1+
import { forceSimulation, forceLink, forceManyBody, forceCenter, forceCollide, forceX, forceY } from 'd3-force'
22
import type { CanvasElement } from '../types.js'
33
import type { LayoutOptions, LayoutResult, LayoutPosition, ForceOptions } from './types.js'
44
import { buildLayoutGraph, computeBoundingBox } from './graph.js'
@@ -52,6 +52,8 @@ export function computeForceLayout(
5252
)
5353
.force('charge', forceManyBody().strength(forceOpts.chargeStrength!))
5454
.force('center', forceCenter(0, 0).strength(forceOpts.centerStrength!))
55+
.force('x', forceX(0).strength(0.05))
56+
.force('y', forceY(0).strength(0.05))
5557
.force(
5658
'collide',
5759
forceCollide().radius((d: any) => Math.max(d.width, d.height) / 2 + 20)

src/layout/graph.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export function buildLayoutGraph(elements: CanvasElement[]): LayoutGraph {
3737
const targetId = el.endElementId
3838

3939
if (!sourceId || !targetId) continue
40+
if (sourceId === targetId) continue
4041
if (!nodeIds.has(sourceId) || !nodeIds.has(targetId)) continue
4142

4243
edges.push({

test/layout.test.ts

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,4 +234,86 @@ describe('SVG Export', () => {
234234

235235
expect(svg).toContain('<line')
236236
})
237+
238+
it('handles empty elements array', () => {
239+
const svg = generateSvg([])
240+
241+
expect(svg).toContain('<svg')
242+
expect(svg).toContain('viewBox="0 0 800 600"')
243+
})
244+
245+
it('escapes XML special characters in text', () => {
246+
const elements = [
247+
makeElement({ id: 'test', text: '<script>alert("xss")</script>' }),
248+
]
249+
250+
const svg = generateSvg(elements)
251+
252+
expect(svg).not.toContain('<script>')
253+
expect(svg).toContain('&lt;script&gt;')
254+
})
255+
})
256+
257+
describe('Layout Engine Edge Cases', () => {
258+
it('handles empty elements array', () => {
259+
const result = computeLayout([])
260+
261+
expect(result.elementCount).toBe(0)
262+
expect(result.positions).toHaveLength(0)
263+
})
264+
265+
it('handles single element', () => {
266+
const elements = [makeElement({ id: 'single' })]
267+
const result = computeLayout(elements)
268+
269+
expect(result.elementCount).toBe(1)
270+
expect(result.positions).toHaveLength(1)
271+
})
272+
273+
it('handles elements with missing dimensions', () => {
274+
const elements = [
275+
makeElement({ id: 'no-dims', width: undefined, height: undefined }),
276+
]
277+
278+
const result = computeLayout(elements)
279+
280+
expect(result.elementCount).toBe(1)
281+
expect(result.positions[0].width).toBe(160)
282+
expect(result.positions[0].height).toBe(80)
283+
})
284+
285+
it('handles self-referencing arrows gracefully', () => {
286+
const elements = [
287+
makeElement({ id: 'self' }),
288+
makeArrow('self', 'self'),
289+
]
290+
291+
const graph = buildLayoutGraph(elements)
292+
293+
expect(graph.nodes).toHaveLength(1)
294+
expect(graph.edges).toHaveLength(0)
295+
})
296+
297+
it('dagre handles disconnected graph', () => {
298+
const elements = [
299+
makeElement({ id: 'a' }),
300+
makeElement({ id: 'b' }),
301+
]
302+
303+
const result = computeDagreLayout(elements)
304+
305+
expect(result.elementCount).toBe(2)
306+
expect(result.bbox.width).toBeGreaterThan(0)
307+
})
308+
309+
it('force layout handles disconnected graph', () => {
310+
const elements = [
311+
makeElement({ id: 'a' }),
312+
makeElement({ id: 'b' }),
313+
]
314+
315+
const result = computeForceLayout(elements)
316+
317+
expect(result.elementCount).toBe(2)
318+
})
237319
})

0 commit comments

Comments
 (0)