Copilot commented on code in PR #64013:
URL: https://github.com/apache/airflow/pull/64013#discussion_r3309069305
##########
airflow/www/static/js/dag/details/taskInstance/Logs/LogBlock.tsx:
##########
@@ -35,25 +40,53 @@ const LogBlock = ({
setUnfoldedLogGroup,
}: Props) => {
const [autoScroll, setAutoScroll] = useState(true);
+ const [logHeight, setLogHeight] = useState<number>();
const logBoxRef = useRef<HTMLPreElement>(null);
- const offsetTop = useOffsetTop(logBoxRef);
+
+ const resizeLogBlock = useCallback(() => {
+ requestAnimationFrame(() => {
+ const logBox = logBoxRef.current;
+ if (!logBox) return;
+
+ const footerHeight =
+ parseInt(getComputedStyle(document.body).paddingBottom, 10) || 0;
+ const viewportHeight = window.visualViewport?.height ??
window.innerHeight;
+ const { top } = logBox.getBoundingClientRect();
+
+ setLogHeight(Math.max(0, viewportHeight - top - footerHeight));
+ });
+ }, []);
const scrollToBottom = () => {
- if (logBoxRef.current) {
- logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
- }
+ requestAnimationFrame(() => {
+ if (logBoxRef.current) {
+ logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
+ }
+ });
};
+ useLayoutEffect(() => {
+ resizeLogBlock();
+
+ window.addEventListener("resize", resizeLogBlock);
+ window.visualViewport?.addEventListener("resize", resizeLogBlock);
+
+ return () => {
+ window.removeEventListener("resize", resizeLogBlock);
+ window.visualViewport?.removeEventListener("resize", resizeLogBlock);
+ };
+ }, [resizeLogBlock, wrap, parsedLogs]);
+
useEffect(() => {
// Always scroll to bottom when wrap or tryNumber change
- if (offsetTop) scrollToBottom();
- }, [wrap, tryNumber, offsetTop]);
+ if (logBoxRef.current) scrollToBottom();
+ }, [wrap, tryNumber, logHeight]);
Review Comment:
Including `logHeight` in this \"always scroll to bottom\" effect will also
force-scroll to the bottom on window/viewport resize (since `logHeight`
changes), even if the user intentionally scrolled up to read earlier logs.
Consider removing `logHeight` from this dependency list, or only scrolling on
height changes when `autoScroll` is enabled (or when the current scroll
position is already near the bottom).
##########
airflow/www/static/js/dag/details/taskInstance/Logs/LogBlock.tsx:
##########
@@ -35,25 +40,53 @@ const LogBlock = ({
setUnfoldedLogGroup,
}: Props) => {
const [autoScroll, setAutoScroll] = useState(true);
+ const [logHeight, setLogHeight] = useState<number>();
const logBoxRef = useRef<HTMLPreElement>(null);
- const offsetTop = useOffsetTop(logBoxRef);
+
+ const resizeLogBlock = useCallback(() => {
+ requestAnimationFrame(() => {
+ const logBox = logBoxRef.current;
+ if (!logBox) return;
+
+ const footerHeight =
+ parseInt(getComputedStyle(document.body).paddingBottom, 10) || 0;
+ const viewportHeight = window.visualViewport?.height ??
window.innerHeight;
+ const { top } = logBox.getBoundingClientRect();
+
+ setLogHeight(Math.max(0, viewportHeight - top - footerHeight));
+ });
+ }, []);
const scrollToBottom = () => {
- if (logBoxRef.current) {
- logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
- }
+ requestAnimationFrame(() => {
+ if (logBoxRef.current) {
+ logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
+ }
+ });
};
+ useLayoutEffect(() => {
+ resizeLogBlock();
+
+ window.addEventListener("resize", resizeLogBlock);
+ window.visualViewport?.addEventListener("resize", resizeLogBlock);
+
+ return () => {
+ window.removeEventListener("resize", resizeLogBlock);
+ window.visualViewport?.removeEventListener("resize", resizeLogBlock);
+ };
+ }, [resizeLogBlock, wrap, parsedLogs]);
Review Comment:
`wrap` and especially `parsedLogs` in the dependency array cause this effect
to tear down and re-add global resize listeners frequently (likely on every log
update), which is unnecessary and can become expensive. Split this into (1) a
mount/unmount effect that only registers listeners once (deps:
`[resizeLogBlock]`), and (2) a separate effect that calls `resizeLogBlock()`
when `wrap` (and/or other layout-affecting inputs) change.
##########
airflow/www/static/js/dag/details/taskInstance/Logs/LogBlock.tsx:
##########
@@ -35,25 +40,53 @@ const LogBlock = ({
setUnfoldedLogGroup,
}: Props) => {
const [autoScroll, setAutoScroll] = useState(true);
+ const [logHeight, setLogHeight] = useState<number>();
const logBoxRef = useRef<HTMLPreElement>(null);
Review Comment:
The ref and event typing don’t match what’s actually rendered: `Code`
renders a `code` element by default (not a `pre`), and the handler types
`HTMLDivElement`. For more accurate typing (and to avoid future ref/handler
misuse), align these to the actual element type used by the scroll container
(e.g., `HTMLElement`/`HTMLCodeElement`, or use Chakra’s `as=\"pre\"` and type
the ref/event accordingly).
##########
airflow/www/static/js/dag/details/taskInstance/Logs/LogBlock.tsx:
##########
@@ -35,25 +40,53 @@ const LogBlock = ({
setUnfoldedLogGroup,
}: Props) => {
const [autoScroll, setAutoScroll] = useState(true);
+ const [logHeight, setLogHeight] = useState<number>();
const logBoxRef = useRef<HTMLPreElement>(null);
- const offsetTop = useOffsetTop(logBoxRef);
+
+ const resizeLogBlock = useCallback(() => {
+ requestAnimationFrame(() => {
+ const logBox = logBoxRef.current;
+ if (!logBox) return;
+
+ const footerHeight =
+ parseInt(getComputedStyle(document.body).paddingBottom, 10) || 0;
+ const viewportHeight = window.visualViewport?.height ??
window.innerHeight;
+ const { top } = logBox.getBoundingClientRect();
+
+ setLogHeight(Math.max(0, viewportHeight - top - footerHeight));
+ });
+ }, []);
const scrollToBottom = () => {
- if (logBoxRef.current) {
- logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
- }
+ requestAnimationFrame(() => {
+ if (logBoxRef.current) {
+ logBoxRef.current.scrollTop = logBoxRef.current.scrollHeight;
+ }
+ });
};
+ useLayoutEffect(() => {
+ resizeLogBlock();
+
+ window.addEventListener("resize", resizeLogBlock);
+ window.visualViewport?.addEventListener("resize", resizeLogBlock);
+
+ return () => {
+ window.removeEventListener("resize", resizeLogBlock);
+ window.visualViewport?.removeEventListener("resize", resizeLogBlock);
+ };
+ }, [resizeLogBlock, wrap, parsedLogs]);
+
useEffect(() => {
// Always scroll to bottom when wrap or tryNumber change
- if (offsetTop) scrollToBottom();
- }, [wrap, tryNumber, offsetTop]);
+ if (logBoxRef.current) scrollToBottom();
+ }, [wrap, tryNumber, logHeight]);
useEffect(() => {
// When logs change, only scroll if autoScroll is enabled
- if (autoScroll && offsetTop) scrollToBottom();
- }, [parsedLogs, autoScroll, offsetTop]);
+ if (autoScroll && logBoxRef.current) scrollToBottom();
+ }, [parsedLogs, autoScroll]);
const onScroll = (e: React.UIEvent<HTMLDivElement>) => {
Review Comment:
The ref and event typing don’t match what’s actually rendered: `Code`
renders a `code` element by default (not a `pre`), and the handler types
`HTMLDivElement`. For more accurate typing (and to avoid future ref/handler
misuse), align these to the actual element type used by the scroll container
(e.g., `HTMLElement`/`HTMLCodeElement`, or use Chakra’s `as=\"pre\"` and type
the ref/event accordingly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
