mirror of
https://github.com/vee1e/mitmproxy.git
synced 2026-09-01 10:18:26 +00:00
fix(web): avoid infinite componentDidUpdate loop in the event log (#8312)
* fix(web): avoid infinite componentDidUpdate loop in the event log EventList.componentDidUpdate called this.onViewportUpdate() on every update, even when the inputs to calcVScroll (the event list and the row height) had not changed. onViewportUpdate reads the current scroll position, recomputes vScroll, and setStates when the window shape changes. Because the event log measures per-row heights, a setState it produced could feed itself: rendering a new window remeasured rows, which shifted the computed window, which setState again, so the cycle never converged and surfaced as "Maximum update depth exceeded" while scrolling the log. Gate the call on prevProps.events !== this.props.events || prevProps.rowHeight !== this.props.rowHeight, mirroring the FlowTable fix in #8233. The other call sites (componentDidMount, the resize listener, the viewport onScroll, and setHeight when a row is first measured) still drive updates as needed, so scrolling and autoscroll keep working. Adds a regression test that fails without the gate. * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Maximilian Hils <git@maximilianhils.com>
This commit is contained in:
parent
a138148e4f
commit
cca8d91b56
3 changed files with 58 additions and 2 deletions
|
|
@ -7,6 +7,8 @@
|
|||
|
||||
## Unreleased: mitmproxy next
|
||||
|
||||
- mitmweb: Fix an infinite update cycle in the event log by only recomputing the virtual-scroll window in `componentDidUpdate` when the event list or `rowHeight` actually change.
|
||||
([#8312](https://github.com/mitmproxy/mitmproxy/pull/8312), @hexbinoct)
|
||||
- Remove the unused `msgpack` dependency. The msgpack contentview is
|
||||
implemented in Rust and shipped with `mitmproxy_rs` since mitmproxy 12.
|
||||
([#8319](https://github.com/mitmproxy/mitmproxy/pull/8319), @lukehsiao)
|
||||
|
|
|
|||
|
|
@ -17,4 +17,43 @@ describe("EventList Component", () => {
|
|||
expect(asFragment()).toMatchSnapshot();
|
||||
unmount(); // no errors
|
||||
});
|
||||
|
||||
it("does not call onViewportUpdate when events and rowHeight are unchanged", () => {
|
||||
// Regression guard for an infinite componentDidUpdate -> setState
|
||||
// cycle (React "Maximum update depth exceeded" when scrolling the
|
||||
// event log). Before the props-comparison gate in componentDidUpdate,
|
||||
// onViewportUpdate ran on EVERY update, including the setState the
|
||||
// previous call itself produced, so with variable row heights the
|
||||
// recomputed vScroll could ping-pong between windows and never
|
||||
// converge. The same fix was applied to FlowTable in #8233.
|
||||
const spy = jest.spyOn(EventLogList.prototype, "onViewportUpdate");
|
||||
const { rerender } = render(<EventLogList events={mockEventList} />);
|
||||
spy.mockClear(); // ignore the componentDidMount call
|
||||
|
||||
// Re-render with the same events reference and rowHeight. This
|
||||
// triggers componentDidUpdate but changes none of onViewportUpdate's
|
||||
// inputs, so it must not run.
|
||||
rerender(<EventLogList events={mockEventList} />);
|
||||
expect(spy).not.toHaveBeenCalled();
|
||||
|
||||
spy.mockRestore();
|
||||
});
|
||||
|
||||
it("calls onViewportUpdate when the events list changes", () => {
|
||||
// Complement of the previous test: a new events array must still
|
||||
// refresh the virtual-scroll window, otherwise appended log lines
|
||||
// would never become visible.
|
||||
const spy = jest.spyOn(EventLogList.prototype, "onViewportUpdate");
|
||||
const { rerender } = render(<EventLogList events={mockEventList} />);
|
||||
spy.mockClear();
|
||||
|
||||
const moreEvents: EventLogItem[] = [
|
||||
...mockEventList,
|
||||
{ id: "3", level: LogLevel.info, message: "baz" },
|
||||
];
|
||||
rerender(<EventLogList events={moreEvents} />);
|
||||
expect(spy).toHaveBeenCalled();
|
||||
|
||||
spy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -49,14 +49,29 @@ export default class EventLogList extends Component<
|
|||
}
|
||||
|
||||
componentDidUpdate(
|
||||
_prevProps: EventLogListProps,
|
||||
prevProps: EventLogListProps,
|
||||
_prevState: EventLogListState,
|
||||
snapshot: boolean,
|
||||
) {
|
||||
if (snapshot) {
|
||||
autoscroll.adjustScrollTop(this.viewport);
|
||||
}
|
||||
this.onViewportUpdate();
|
||||
// Only recompute the virtual-scroll window when the event list or the
|
||||
// row height actually changed. Calling onViewportUpdate on every
|
||||
// update (including the setState it produces itself) let
|
||||
// setState -> componentDidUpdate -> setState spin without converging
|
||||
// once measured row heights differed from the assumed rowHeight,
|
||||
// surfacing as "Maximum update depth exceeded" while scrolling the
|
||||
// event log. The same fix was applied to FlowTable in #8233. The
|
||||
// other call sites still drive updates as needed: componentDidMount,
|
||||
// the resize listener, the viewport onScroll, and setHeight when a
|
||||
// row is first measured.
|
||||
if (
|
||||
prevProps.events !== this.props.events ||
|
||||
prevProps.rowHeight !== this.props.rowHeight
|
||||
) {
|
||||
this.onViewportUpdate();
|
||||
}
|
||||
}
|
||||
|
||||
onViewportUpdate() {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue