fix(client): re-anchor the trend chart only on real data pushes
The chart's layout effect ran after every render (no deps), so any hover/select re-render near the right edge re-ran the stick-to-end check — and by the time it read el.scrollWidth the value was already the post-append width, which could misfire the anchor. The effect now runs only when granularity, focusTurn, or the request list changes, and the near-end check compares against the PREVIOUS pass's scrollWidth (kept in a ref), so a reader near the right edge still follows an appended bar while a mid-chart reader is never yanked back, and a hover/select render never touches the scroll at all.
This commit is contained in:
1 parent
1b2590e0db
commit
01d16bbcf0
2 files changed
+56
-14
No files matched your search
@@ -251,10 +251,17 @@ export function makeTrendChart(kit: ViewKit): (props: TrendChartProps) => ReactN
|
||||
const scrollRef = React.useRef<HTMLDivElement | null>(null)
|
||||
const scrolledOnce = React.useRef(false)
|
||||
const lastGranRef = React.useRef(props.granularity)
|
||||
// The newest bar's seq (or 0 when the log is empty): the layout effect only re-runs when the right edge genuinely
|
||||
// moves (new bar appended, granularity switched, focus turn set) — hover/select changes keep their scroll position
|
||||
// so the chart does not flash with every keystroke.
|
||||
const lastSeqRef = React.useRef(0)
|
||||
// The scrollWidth measured during the PREVIOUS effect pass. The "was the reader near the right edge?" check
|
||||
// has to compare against the width as it was BEFORE the new bar landed — by the time the layout effect runs,
|
||||
// `el.scrollWidth` is already the new (wider) value, so a near-edge check against it would miss the auto-follow.
|
||||
const prevScrollWidthRef = React.useRef(0)
|
||||
const [edges, setEdges] = React.useState<{ left: boolean; right: boolean }>({ left: false, right: false })
|
||||
// Mirror of the last computed fades: the layout effect runs after EVERY render (no deps), so it must dispatch setState only on true
|
||||
// change — a same-value dispatch during a granularity switch's pending lanes disables React's eager bailout and the queue grows
|
||||
// unbounded (React error #185).
|
||||
// Mirror of the last computed fades: the layout effect dispatches setState only on true change, so a same-value
|
||||
// dispatch during a granularity switch's pending lanes does not disable React's eager bailout (React error #185).
|
||||
const edgesRef = React.useRef(edges)
|
||||
const updateEdges = (el: HTMLDivElement): void => {
|
||||
const left = el.scrollLeft > 4
|
||||
@@ -298,6 +305,9 @@ export function makeTrendChart(kit: ViewKit): (props: TrendChartProps) => ReactN
|
||||
/* v8 ignore next 1 -- the scroll div renders unconditionally and React
|
||||
attaches refs before layout effects run; el is never null here. */
|
||||
if (el === null) return
|
||||
const newestSeq = requests.length === 0 ? 0 : requests[requests.length - 1].seq
|
||||
const grew = newestSeq !== lastSeqRef.current
|
||||
const widthBeforeAppend = prevScrollWidthRef.current
|
||||
if (props.granularity !== lastGranRef.current) {
|
||||
lastGranRef.current = props.granularity
|
||||
scrolledOnce.current = false
|
||||
@@ -311,16 +321,20 @@ export function makeTrendChart(kit: ViewKit): (props: TrendChartProps) => ReactN
|
||||
el.scrollLeft = Math.max(0, gi * (BAR_W + BAR_GAP) + BAR_W / 2 - el.clientWidth / 2)
|
||||
}
|
||||
props.onFocusTurnHandled()
|
||||
}
|
||||
if (!scrolledOnce.current) {
|
||||
} else if (!scrolledOnce.current) {
|
||||
scrolledOnce.current = true
|
||||
el.scrollLeft = el.scrollWidth
|
||||
} else if (el.scrollLeft + el.clientWidth >= el.scrollWidth - 24) {
|
||||
} else if (grew && el.scrollLeft + el.clientWidth >= widthBeforeAppend - 24) {
|
||||
// Only follow the latest bar when the right edge actually moved AND the reader was already near it
|
||||
// BEFORE the new bar landed — comparing against the new scrollWidth would silently drop the stick for a
|
||||
// mid-chart reader whose viewport just slid past the near-end threshold.
|
||||
el.scrollLeft = el.scrollWidth
|
||||
}
|
||||
lastSeqRef.current = newestSeq
|
||||
prevScrollWidthRef.current = el.scrollWidth
|
||||
updateEdges(el)
|
||||
updateTurnLabels(el)
|
||||
})
|
||||
}, [props.granularity, props.focusTurn, requests])
|
||||
|
||||
// Compact single-line hover tooltip, shown instantly by the custom `.lc-chart-tip` (the native title is delayed): position, time,
|
||||
// estimated total, provider prompt when available; the per-category breakdown lives in the detail panel below.
|
||||
|
||||
@@ -501,19 +501,21 @@ describe('TrendChart edge fades and scroll anchoring', () => {
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-l').length, 1)
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-r').length, 1)
|
||||
|
||||
// An unrelated update mid-scroll does NOT re-anchor (100 + 400 < 640 - 24).
|
||||
// An unrelated (selection-only) update mid-scroll does NOT re-anchor (100 + 400 < 640 - 24), and the chart
|
||||
// also stays put when the same selection-only update fires NEAR the right edge — only a real data push
|
||||
// (a new request appended, or a granularity/focus switch) may follow the newest bar, so a hover/select
|
||||
// change never flashes the column.
|
||||
await m.update(h(TrendChart, propsOf(reqs, { ...handlers, selectedSeq: 1 })))
|
||||
assert.equal(scroll.scrollLeft, 100)
|
||||
// …but near the end (230 + 400 >= 640 - 24) the next render sticks to the newest edge.
|
||||
await scrollTo(scroll, 230)
|
||||
await m.update(h(TrendChart, propsOf(reqs, { ...handlers, selectedSeq: 2 })))
|
||||
await flush()
|
||||
assert.equal(scroll.scrollLeft, 240)
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-l').length, 1)
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-r').length, 0)
|
||||
assert.equal(scroll.scrollLeft, 230, 'selection-only update near the end does not stick')
|
||||
|
||||
// Turn labels re-center within their visible slice: T2 is half-clipped at the left → shifted right;
|
||||
// T3 is fully visible and centered → no transform; T1 is fully out of view → untouched.
|
||||
// Turn labels re-center within their visible slice: with the reader at scrollLeft 240, T2 is half-clipped
|
||||
// at the left → shifted right; T3 is fully visible and centered → no transform; T1 is fully out of view
|
||||
// → untouched.
|
||||
await scrollTo(scroll, 240)
|
||||
const labels = queryAll(m.container, '.lc-turn-label')
|
||||
assert.equal(labels.length, 4)
|
||||
assert.equal(labels[1].style.transform, 'translateX(40px)')
|
||||
@@ -524,6 +526,32 @@ describe('TrendChart edge fades and scroll anchoring', () => {
|
||||
await m.unmount()
|
||||
})
|
||||
|
||||
test('a data push follows the newest bar only when the reader was near the right edge', async () => {
|
||||
const reqs = manySteps()
|
||||
const { handlers } = makeSpies()
|
||||
const m = await mount(h(TrendChart, propsOf(reqs, handlers)))
|
||||
await flush()
|
||||
const scroll = query<LayoutEl>(m.container, '.lc-chart-scroll')
|
||||
// Mount anchors at the right edge (scrollLeft = 240 over scrollWidth 640 / clientWidth 400).
|
||||
assert.equal(scroll.scrollLeft, 240)
|
||||
|
||||
// A new request appended while the reader sits at the right edge → follows the newest bar.
|
||||
const grown = [...reqs, req(reqs.length + 1, { turn: 1 + Math.floor(reqs.length / 10), step: reqs.length % 10 })]
|
||||
await m.update(h(TrendChart, propsOf(grown, handlers)))
|
||||
await flush()
|
||||
assert.equal(scroll.scrollLeft, 256, 'near-edge reader follows the appended bar')
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-l').length, 1)
|
||||
assert.equal(queryAll(m.container, '.lc-chart-fade-r').length, 0)
|
||||
|
||||
// A push while the reader is mid-chart leaves them where they were — no auto-follow yank.
|
||||
await scrollTo(scroll, 100)
|
||||
const grown2 = [...grown, req(grown.length + 1, { turn: 1 + Math.floor(grown.length / 10), step: grown.length % 10 })]
|
||||
await m.update(h(TrendChart, propsOf(grown2, handlers)))
|
||||
await flush()
|
||||
assert.equal(scroll.scrollLeft, 100, 'mid-scroll reader is not yanked back to the newest')
|
||||
await m.unmount()
|
||||
})
|
||||
|
||||
test('a granularity switch re-anchors and flips overflow for real (React #185 regression)', async () => {
|
||||
const reqs = manySteps()
|
||||
const { handlers } = makeSpies()
|
||||
|
||||
Reference in new issue
Block a user