Fix review perf issues.
This commit is contained in:
+10
-11
@@ -373,21 +373,20 @@ export default function App() {
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
// Live updates. Every event on this stream is about this review — the stream is
|
// Live updates. Every event on this stream is about this review — the stream is
|
||||||
// the tab's own — so anything that isn't the opening handshake means the comment
|
// the tab's own — so anything that arrives means the comment list moved and is
|
||||||
// list moved and is worth refetching.
|
// worth refetching.
|
||||||
|
//
|
||||||
|
// The handshake refetches too, which it did not used to. The stream is dropped
|
||||||
|
// while the pane is hidden (see useSSE) and reconnects when it comes back, so
|
||||||
|
// `connected` is the one moment the rail is known to be behind: every comment
|
||||||
|
// written while this pane was off screen is a frame nobody was listening for.
|
||||||
|
// Treating the handshake as "something changed" is what closes that gap — and
|
||||||
|
// the same gap after a dropped connection, which was always there.
|
||||||
//
|
//
|
||||||
// Comments only. The diff is not pushed: the server has no watcher on the work
|
// Comments only. The diff is not pushed: the server has no watcher on the work
|
||||||
// tree, and a comment arriving is a change to the rail, which can be applied
|
// tree, and a comment arriving is a change to the rail, which can be applied
|
||||||
// under the reader safely. A changed *diff* cannot — see the poll below.
|
// under the reader safely. A changed *diff* cannot — see the poll below.
|
||||||
useSSE(
|
useSSE(useCallback(() => refetchComments(), [refetchComments]));
|
||||||
useCallback(
|
|
||||||
(e) => {
|
|
||||||
if (e.type === 'connected') return;
|
|
||||||
refetchComments();
|
|
||||||
},
|
|
||||||
[refetchComments],
|
|
||||||
),
|
|
||||||
);
|
|
||||||
|
|
||||||
// Notice that the diff on screen has been overtaken, and say so. Nothing here
|
// Notice that the diff on screen has been overtaken, and say so. Nothing here
|
||||||
// reloads anything: it sets `stale`, the banner offers the refresh, and the
|
// reloads anything: it sets `stale`, the banner offers the refresh, and the
|
||||||
|
|||||||
+36
-5
@@ -13,6 +13,22 @@ export interface ServerEvent {
|
|||||||
// The stream is scoped by the URL it is opened on — the tab's own `api/events` —
|
// The stream is scoped by the URL it is opened on — the tab's own `api/events` —
|
||||||
// so unlike the tool this came from there is nothing to filter here: activity in
|
// so unlike the tool this came from there is nothing to filter here: activity in
|
||||||
// another tab's review never arrives in the first place.
|
// another tab's review never arrives in the first place.
|
||||||
|
//
|
||||||
|
// ## Why this closes while the pane is hidden
|
||||||
|
//
|
||||||
|
// Every pane is a webview onto the same origin, and they all share one WebKit
|
||||||
|
// network session — so they also share its cap of six HTTP/1.1 connections per
|
||||||
|
// host. A stream held open for the pane's whole lifetime is one of those six
|
||||||
|
// spent permanently, and with a review open in seven tabs the panes starve each
|
||||||
|
// other: the diff, comment and repo fetches queue behind streams that are only
|
||||||
|
// ever going to sit there, and a small diff that the server answers in
|
||||||
|
// milliseconds never loads at all.
|
||||||
|
//
|
||||||
|
// So a hidden pane gives its connection back, exactly as the diff poll gives up
|
||||||
|
// its timer, and takes one again when it comes back on screen. What that costs
|
||||||
|
// is the events that arrived while it was away, which is why reconnecting
|
||||||
|
// resyncs the comment list rather than assuming the rail is still current — see
|
||||||
|
// the `connected` handler in App.
|
||||||
export function useSSE(onEvent: (e: ServerEvent) => void): void {
|
export function useSSE(onEvent: (e: ServerEvent) => void): void {
|
||||||
const handler = useRef(onEvent);
|
const handler = useRef(onEvent);
|
||||||
handler.current = onEvent;
|
handler.current = onEvent;
|
||||||
@@ -22,8 +38,17 @@ export function useSSE(onEvent: (e: ServerEvent) => void): void {
|
|||||||
let closed = false;
|
let closed = false;
|
||||||
let retry: ReturnType<typeof setTimeout> | undefined;
|
let retry: ReturnType<typeof setTimeout> | undefined;
|
||||||
|
|
||||||
|
const disconnect = () => {
|
||||||
|
if (retry) {
|
||||||
|
clearTimeout(retry);
|
||||||
|
retry = undefined;
|
||||||
|
}
|
||||||
|
es?.close();
|
||||||
|
es = null;
|
||||||
|
};
|
||||||
|
|
||||||
const connect = () => {
|
const connect = () => {
|
||||||
if (closed) return;
|
if (closed || es || document.hidden) return;
|
||||||
es = new EventSource(`${apiBase}/events`);
|
es = new EventSource(`${apiBase}/events`);
|
||||||
// The server's opening nudge is an SSE comment, so it never reaches
|
// The server's opening nudge is an SSE comment, so it never reaches
|
||||||
// onmessage; synthesize it, so a caller that wants to know the stream is up
|
// onmessage; synthesize it, so a caller that wants to know the stream is up
|
||||||
@@ -37,16 +62,22 @@ export function useSSE(onEvent: (e: ServerEvent) => void): void {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
es.onerror = () => {
|
es.onerror = () => {
|
||||||
es?.close();
|
disconnect();
|
||||||
if (!closed) retry = setTimeout(connect, 2000);
|
if (!closed && !document.hidden) retry = setTimeout(connect, 2000);
|
||||||
};
|
};
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const onVisibility = () => {
|
||||||
|
if (document.hidden) disconnect();
|
||||||
|
else connect();
|
||||||
|
};
|
||||||
|
|
||||||
connect();
|
connect();
|
||||||
|
document.addEventListener('visibilitychange', onVisibility);
|
||||||
return () => {
|
return () => {
|
||||||
closed = true;
|
closed = true;
|
||||||
if (retry) clearTimeout(retry);
|
document.removeEventListener('visibilitychange', onVisibility);
|
||||||
es?.close();
|
disconnect();
|
||||||
};
|
};
|
||||||
}, []);
|
}, []);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user