Skip to content

Commit 7507fbd

Browse files
cmdcolincmdcolin
authored
Optimize gridlines component (#5196)
1 parent b8de40e commit 7507fbd

File tree

5 files changed

+2672
-1290
lines changed

5 files changed

+2672
-1290
lines changed

‎plugins/linear-genome-view/src/LinearBasicDisplay/model.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { lazy } from 'react'
33
import { ConfigurationReference, getConf } from '@jbrowse/core/configuration'
44
import SerializableFilterChain from '@jbrowse/core/pluggableElementTypes/renderers/util/serializableFilterChain'
55
import { getSession } from '@jbrowse/core/util'
6+
import FilterAltIcon from '@mui/icons-material/FilterAlt'
67
import VisibilityIcon from '@mui/icons-material/Visibility'
78
import { cast, getEnv, types } from 'mobx-state-tree'
89

@@ -238,6 +239,7 @@ function stateModelFactory(configSchema: AnyConfigurationSchemaType) {
238239
},
239240
{
240241
label: 'Edit filters',
242+
icon: FilterAltIcon,
241243
onClick: () => {
242244
getSession(self).queueDialog(handleClose => [
243245
AddFiltersDialog,

‎plugins/linear-genome-view/src/LinearGenomeView/components/Gridlines.tsx‎

Lines changed: 209 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
import { memo, useEffect, useMemo, useRef } from 'react'
2+
3+
import { useTheme } from '@mui/material'
4+
import { reaction } from 'mobx'
15
import { observer } from 'mobx-react'
26
import { makeStyles } from 'tss-react/mui'
37

@@ -20,17 +24,20 @@ const useStyles = makeStyles()(theme => ({
2024
height: '100%',
2125
width: '100%',
2226
pointerEvents: 'none',
27+
contain: 'layout style paint',
2328
},
2429
verticalGuidesContainer: {
2530
position: 'absolute',
2631
height: '100%',
2732
pointerEvents: 'none',
2833
display: 'flex',
34+
contain: 'layout style',
2935
},
3036
tick: {
3137
position: 'absolute',
3238
height: '100%',
3339
width: 1,
40+
willChange: 'transform',
3441
},
3542
majorTick: {
3643
background: theme.palette.action.disabled,
@@ -40,60 +47,175 @@ const useStyles = makeStyles()(theme => ({
4047
},
4148
}))
4249

43-
function RenderedBlockLines({
44-
block,
45-
bpPerPx,
46-
}: {
47-
block: ContentBlock
48-
bpPerPx: number
49-
}) {
50-
const { classes, cx } = useStyles()
51-
const ticks = makeTicks(block.start, block.end, bpPerPx)
52-
return (
53-
<ContentBlockComponent block={block}>
54-
{ticks.map(({ type, base }) => {
50+
const RenderedBlockLines = memo(
51+
function RenderedBlockLines({
52+
block,
53+
bpPerPx,
54+
majorColor,
55+
minorColor,
56+
}: {
57+
block: ContentBlock
58+
bpPerPx: number
59+
majorColor: string
60+
minorColor: string
61+
}) {
62+
const svgRef = useRef<SVGSVGElement>(null)
63+
const lastRenderedKey = useRef<string>('')
64+
65+
// Update SVG lines directly without React, with manual caching
66+
useEffect(() => {
67+
const svg = svgRef.current
68+
if (!svg) {
69+
return
70+
}
71+
72+
// Create a cache key based on actual values
73+
const cacheKey = `${block.key}-${block.start}-${block.end}-${block.reversed}-${bpPerPx}-${majorColor}-${minorColor}`
74+
75+
// Skip if nothing actually changed
76+
if (lastRenderedKey.current === cacheKey) {
77+
return
78+
}
79+
80+
lastRenderedKey.current = cacheKey
81+
82+
const ticks = makeTicks(block.start, block.end, bpPerPx)
83+
84+
// Clear existing lines
85+
svg.innerHTML = ''
86+
87+
// Create lines directly in SVG
88+
const fragment = document.createDocumentFragment()
89+
for (const { type, base } of ticks) {
5590
const x =
5691
(block.reversed ? block.end - base : base - block.start) / bpPerPx
57-
return (
58-
<div
59-
key={base}
60-
className={cx(
61-
classes.tick,
62-
type === 'major' || type === 'labeledMajor'
63-
? classes.majorTick
64-
: classes.minorTick,
65-
)}
66-
style={{ left: x }}
67-
/>
92+
const line = document.createElementNS(
93+
'http://www.w3.org/2000/svg',
94+
'line',
95+
)
96+
line.setAttribute('x1', String(x))
97+
line.setAttribute('y1', '0')
98+
line.setAttribute('x2', String(x))
99+
line.setAttribute('y2', '100%')
100+
line.setAttribute(
101+
'stroke',
102+
type === 'major' || type === 'labeledMajor' ? majorColor : minorColor,
68103
)
69-
})}
70-
</ContentBlockComponent>
104+
line.setAttribute('stroke-width', '1')
105+
fragment.append(line)
106+
}
107+
svg.append(fragment)
108+
}) // No dependencies - runs every render but has manual cache check
109+
110+
return (
111+
<ContentBlockComponent block={block}>
112+
<svg
113+
ref={svgRef}
114+
style={{
115+
position: 'absolute',
116+
top: 0,
117+
left: 0,
118+
width: '100%',
119+
height: '100%',
120+
pointerEvents: 'none',
121+
}}
122+
/>
123+
</ContentBlockComponent>
124+
)
125+
},
126+
(prevProps, nextProps) => {
127+
// Only re-render if block key or bpPerPx actually changes
128+
return (
129+
prevProps.block.key === nextProps.block.key &&
130+
prevProps.bpPerPx === nextProps.bpPerPx &&
131+
prevProps.majorColor === nextProps.majorColor &&
132+
prevProps.minorColor === nextProps.minorColor
133+
)
134+
},
135+
)
136+
137+
const RenderedVerticalGuides = observer(function ({ model }: { model: LGV }) {
138+
const { coarseStaticBlocks, bpPerPx } = model
139+
const theme = useTheme()
140+
141+
// Memoize theme colors to prevent unnecessary recalculations
142+
const { majorColor, minorColor } = useMemo(
143+
() => ({
144+
majorColor: theme.palette.action.disabled,
145+
minorColor: theme.palette.divider,
146+
}),
147+
[theme.palette.action.disabled, theme.palette.divider],
71148
)
72-
}
73-
const RenderedVerticalGuides = observer(({ model }: { model: LGV }) => {
74-
const { staticBlocks, bpPerPx } = model
75-
return (
76-
<>
77-
{staticBlocks.map((block, index) => {
78-
const k = `${block.key}-${index}`
79-
if (block.type === 'ContentBlock') {
80-
return <RenderedBlockLines key={k} block={block} bpPerPx={bpPerPx} />
81-
} else if (block.type === 'ElidedBlock') {
82-
return <ElidedBlockComponent key={k} width={block.widthPx} />
83-
} else if (block.type === 'InterRegionPaddingBlock') {
84-
return (
85-
<InterRegionPaddingBlockComponent
86-
key={k}
87-
width={block.widthPx}
88-
boundary={block.variant === 'boundary'}
89-
/>
90-
)
91-
}
92-
return null
93-
})}
94-
</>
149+
150+
// Create stable key to prevent unnecessary re-renders when blocks haven't changed
151+
const blocksKey = useMemo(
152+
() =>
153+
coarseStaticBlocks
154+
? coarseStaticBlocks.map(b => `${b.key}-${b.widthPx}`).join(',')
155+
: '',
156+
[coarseStaticBlocks],
157+
)
158+
159+
// Memoize block elements to prevent recreation on every render
160+
// Note: blocksKey changes only when actual blocks change, not when coarseStaticBlocks reference changes
161+
const blockElements = useMemo(
162+
() =>
163+
coarseStaticBlocks ? (
164+
<>
165+
{coarseStaticBlocks.map((block, index) => {
166+
const k = `${block.key}-${index}`
167+
if (block.type === 'ContentBlock') {
168+
return (
169+
<RenderedBlockLines
170+
key={k}
171+
block={block}
172+
bpPerPx={bpPerPx}
173+
majorColor={majorColor}
174+
minorColor={minorColor}
175+
/>
176+
)
177+
} else if (block.type === 'ElidedBlock') {
178+
return <ElidedBlockComponent key={k} width={block.widthPx} />
179+
} else if (block.type === 'InterRegionPaddingBlock') {
180+
return (
181+
<InterRegionPaddingBlockComponent
182+
key={k}
183+
width={block.widthPx}
184+
boundary={block.variant === 'boundary'}
185+
/>
186+
)
187+
}
188+
return null
189+
})}
190+
</>
191+
) : null,
192+
// Claude Code reasoning for the disable:
193+
//
194+
// The Pattern:
195+
//
196+
// const blocksKey = useMemo(() =>
197+
// coarseStaticBlocks?.map(b => `${b.key}-${b.widthPx}`).join(','),
198+
// [coarseStaticBlocks]
199+
// )
200+
//
201+
// const blockElements = useMemo(() => {
202+
// // Use coarseStaticBlocks here
203+
// }, [blocksKey, ...]) // But depend on blocksKey, not coarseStaticBlocks
204+
//
205+
// This is a legitimate use case for eslint-disable because:
206+
// - We have a derived value (blocksKey) that better represents when to update
207+
// - The linter can't understand this semantic relationship
208+
// - The disable is narrow in scope and well-documented
209+
//
210+
// The alternative (using refs) adds complexity without benefit
211+
//
212+
// eslint-disable-next-line react-hooks/exhaustive-deps
213+
[blocksKey, bpPerPx, majorColor, minorColor],
95214
)
215+
216+
return blockElements
96217
})
218+
97219
const Gridlines = observer(function ({
98220
model,
99221
offset = 0,
@@ -102,23 +224,46 @@ const Gridlines = observer(function ({
102224
offset?: number
103225
}) {
104226
const { classes } = useStyles()
105-
// find the block that needs pinning to the left side for context
106-
const offsetLeft = model.staticBlocks.offsetPx - model.offsetPx
227+
const containerRef = useRef<HTMLDivElement>(null)
228+
const guidesRef = useRef<HTMLDivElement>(null)
229+
230+
// Use MobX reaction to update DOM directly without triggering React re-renders
231+
useEffect(() => {
232+
const container = containerRef.current
233+
const guides = guidesRef.current
234+
if (!container || !guides) {
235+
return
236+
}
237+
238+
// MobX reaction tracks observables and updates DOM directly
239+
const disposer = reaction(
240+
() => ({
241+
scaleFactor: model.scaleFactor,
242+
offsetPx: model.offsetPx,
243+
blocksOffsetPx: model.coarseStaticBlocks?.offsetPx,
244+
totalWidthPx: model.coarseStaticBlocks?.totalWidthPx,
245+
}),
246+
({ scaleFactor, offsetPx, blocksOffsetPx, totalWidthPx }) => {
247+
// Update scale transform
248+
container.style.transform =
249+
scaleFactor !== 1 ? `scaleX(${scaleFactor})` : ''
250+
251+
// Update position and width
252+
if (blocksOffsetPx !== undefined && totalWidthPx !== undefined) {
253+
const translateX = blocksOffsetPx - offsetPx - offset
254+
guides.style.transform = `translateX(${translateX}px)`
255+
guides.style.width = `${totalWidthPx}px`
256+
}
257+
},
258+
{ fireImmediately: true },
259+
)
260+
261+
return disposer
262+
}, [model, offset])
263+
107264
return (
108-
<div
109-
className={classes.verticalGuidesZoomContainer}
110-
style={{
111-
transform:
112-
model.scaleFactor !== 1 ? `scaleX(${model.scaleFactor})` : undefined,
113-
}}
114-
>
115-
<div
116-
className={classes.verticalGuidesContainer}
117-
style={{
118-
left: offsetLeft - offset,
119-
width: model.staticBlocks.totalWidthPx,
120-
}}
121-
>
265+
<div ref={containerRef} className={classes.verticalGuidesZoomContainer}>
266+
<div ref={guidesRef} className={classes.verticalGuidesContainer}>
122267
<RenderedVerticalGuides model={model} />
123268
</div>
124269
</div>

0 commit comments

Comments
 (0)