diff --git a/CHANGELOG.md b/CHANGELOG.md index 683315bc2..baa6ca56b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/web/src/js/__tests__/components/EventLog/EventListSpec.tsx b/web/src/js/__tests__/components/EventLog/EventListSpec.tsx index b6362821d..892a6e062 100644 --- a/web/src/js/__tests__/components/EventLog/EventListSpec.tsx +++ b/web/src/js/__tests__/components/EventLog/EventListSpec.tsx @@ -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(); + 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(); + 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(); + spy.mockClear(); + + const moreEvents: EventLogItem[] = [ + ...mockEventList, + { id: "3", level: LogLevel.info, message: "baz" }, + ]; + rerender(); + expect(spy).toHaveBeenCalled(); + + spy.mockRestore(); + }); }); diff --git a/web/src/js/components/EventLog/EventList.tsx b/web/src/js/components/EventLog/EventList.tsx index d0b0e130d..c59a47bdb 100644 --- a/web/src/js/components/EventLog/EventList.tsx +++ b/web/src/js/components/EventLog/EventList.tsx @@ -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() {