pierrejeambrun commented on code in PR #54447:
URL: https://github.com/apache/airflow/pull/54447#discussion_r2315894759
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogContent.tsx:
##########
@@ -95,15 +96,103 @@ export const TaskLogContent = ({ error, isLoading,
logError, parsedLogs, wrap }:
}, [rowVirtualizer, parsedLogs]);
useLayoutEffect(() => {
- if (location.hash && !isLoading) {
- rowVirtualizer.scrollToIndex(Math.min(Number(hash) + 5,
parsedLogs.length - 1));
+ if (!location.hash || isLoading || parsedLogs.length === 0) {
+ return;
+ }
+
+ const targetIndex = Math.max(0, Math.min(parsedLogs.length - 1,
Number(hash) || 0));
+ const el = parentRef.current;
+
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el?.clientHeight ?? 0;
Review Comment:
Both clicking on a log line, or refreshing the page with a URL with a line
number specified will not land where we want.
http://localhost:28080/dags/big_logs/runs/manual__2025-09-02T12:02:44.060539+00:00/tasks/my_task?try_number=1#4806
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogContent.tsx:
##########
@@ -95,15 +96,103 @@ export const TaskLogContent = ({ error, isLoading,
logError, parsedLogs, wrap }:
}, [rowVirtualizer, parsedLogs]);
useLayoutEffect(() => {
- if (location.hash && !isLoading) {
- rowVirtualizer.scrollToIndex(Math.min(Number(hash) + 5,
parsedLogs.length - 1));
+ if (!location.hash || isLoading || parsedLogs.length === 0) {
+ return;
+ }
+
+ const targetIndex = Math.max(0, Math.min(parsedLogs.length - 1,
Number(hash) || 0));
+ const el = parentRef.current;
+
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el?.clientHeight ?? 0;
+
+ const vItem = rowVirtualizer.getVirtualItems().find((virtualRow) =>
virtualRow.index === targetIndex);
+ const approxPerItem = 20;
+ const anchor = vItem?.start ?? targetIndex * approxPerItem;
+
+ const offset = Math.max(0, Math.min(total - clientH, anchor));
+
+ if (el) {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(offset);
+ } catch {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+
+ el.scrollTop = offset;
+
+ requestAnimationFrame(() => {
+ el.scrollTop = offset;
+ });
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
}
}, [isLoading, rowVirtualizer, hash, parsedLogs]);
const handleScrollTo = (to: "bottom" | "top") => {
- if (parsedLogs.length > 0) {
- rowVirtualizer.scrollToIndex(to === "bottom" ? parsedLogs.length - 1 :
0);
+ if (parsedLogs.length === 0) {
+ return;
+ }
+
+ const el = rowVirtualizer.scrollElement ?? parentRef.current;
+
+ if (!el) {
+ return;
}
+
+ if (to === "top") {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(0);
+ } catch {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ el.scrollTop = 0;
+ requestAnimationFrame(() => {
+ el.scrollTop = 0;
+ });
+
+ return;
+ }
+
+ // === bottom === (instant jump even for huge lists)
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el.clientHeight || 0;
+ const offset = Math.max(0, Math.floor(total - clientH));
+
+ // First tell the virtualizer where we want to be
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(offset);
+ } catch {
+ rowVirtualizer.scrollToIndex(parsedLogs.length - 1, { align: "end" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(parsedLogs.length - 1, { align: "end" });
+ }
+
+ el.scrollTop = offset;
+
+ requestAnimationFrame(() => {
+ el.scrollTop = offset;
+ requestAnimationFrame(() => {
+ el.scrollTop = offset;
+ const lastItem = el.querySelector<HTMLElement>(
+ `[data-testid="virtualized-item-${parsedLogs.length - 1}"]`,
+ );
+
+ if (lastItem) {
+ lastItem.scrollIntoView({ behavior: "auto", block: "end" });
+ }
+ });
+ });
Review Comment:
What are those `requestAnimationFrame` achieving? I Tried without them and
it still look fine.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogContent.tsx:
##########
@@ -95,15 +96,103 @@ export const TaskLogContent = ({ error, isLoading,
logError, parsedLogs, wrap }:
}, [rowVirtualizer, parsedLogs]);
useLayoutEffect(() => {
- if (location.hash && !isLoading) {
- rowVirtualizer.scrollToIndex(Math.min(Number(hash) + 5,
parsedLogs.length - 1));
+ if (!location.hash || isLoading || parsedLogs.length === 0) {
+ return;
+ }
+
+ const targetIndex = Math.max(0, Math.min(parsedLogs.length - 1,
Number(hash) || 0));
+ const el = parentRef.current;
+
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el?.clientHeight ?? 0;
Review Comment:
This breaks the direct link to lines. We used to add 5 more lines so it
wouldn't be on the `edge` of the container, f screenshot.
<img width="1893" height="807" alt="Screenshot 2025-09-02 at 14 06 18"
src="https://github.com/user-attachments/assets/3f5921e7-17bb-4b84-9bff-bb1976ed5fa2";
/>
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogContent.tsx:
##########
@@ -95,15 +96,103 @@ export const TaskLogContent = ({ error, isLoading,
logError, parsedLogs, wrap }:
}, [rowVirtualizer, parsedLogs]);
useLayoutEffect(() => {
- if (location.hash && !isLoading) {
- rowVirtualizer.scrollToIndex(Math.min(Number(hash) + 5,
parsedLogs.length - 1));
+ if (!location.hash || isLoading || parsedLogs.length === 0) {
+ return;
+ }
+
+ const targetIndex = Math.max(0, Math.min(parsedLogs.length - 1,
Number(hash) || 0));
+ const el = parentRef.current;
+
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el?.clientHeight ?? 0;
+
+ const vItem = rowVirtualizer.getVirtualItems().find((virtualRow) =>
virtualRow.index === targetIndex);
+ const approxPerItem = 20;
+ const anchor = vItem?.start ?? targetIndex * approxPerItem;
+
+ const offset = Math.max(0, Math.min(total - clientH, anchor));
+
+ if (el) {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(offset);
+ } catch {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+
+ el.scrollTop = offset;
+
+ requestAnimationFrame(() => {
+ el.scrollTop = offset;
+ });
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
}
}, [isLoading, rowVirtualizer, hash, parsedLogs]);
const handleScrollTo = (to: "bottom" | "top") => {
- if (parsedLogs.length > 0) {
- rowVirtualizer.scrollToIndex(to === "bottom" ? parsedLogs.length - 1 :
0);
+ if (parsedLogs.length === 0) {
+ return;
+ }
+
+ const el = rowVirtualizer.scrollElement ?? parentRef.current;
+
+ if (!el) {
+ return;
}
+
+ if (to === "top") {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(0);
+ } catch {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ el.scrollTop = 0;
+ requestAnimationFrame(() => {
+ el.scrollTop = 0;
+ });
+
+ return;
+ }
+
+ // === bottom === (instant jump even for huge lists)
Review Comment:
There's a comment for bottom, but not for top. Remove it
```suggestion
```
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/Logs/TaskLogContent.tsx:
##########
@@ -95,15 +96,103 @@ export const TaskLogContent = ({ error, isLoading,
logError, parsedLogs, wrap }:
}, [rowVirtualizer, parsedLogs]);
useLayoutEffect(() => {
- if (location.hash && !isLoading) {
- rowVirtualizer.scrollToIndex(Math.min(Number(hash) + 5,
parsedLogs.length - 1));
+ if (!location.hash || isLoading || parsedLogs.length === 0) {
+ return;
+ }
+
+ const targetIndex = Math.max(0, Math.min(parsedLogs.length - 1,
Number(hash) || 0));
+ const el = parentRef.current;
+
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el?.clientHeight ?? 0;
+
+ const vItem = rowVirtualizer.getVirtualItems().find((virtualRow) =>
virtualRow.index === targetIndex);
+ const approxPerItem = 20;
+ const anchor = vItem?.start ?? targetIndex * approxPerItem;
+
+ const offset = Math.max(0, Math.min(total - clientH, anchor));
+
+ if (el) {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(offset);
+ } catch {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
+ }
+
+ el.scrollTop = offset;
+
+ requestAnimationFrame(() => {
+ el.scrollTop = offset;
+ });
+ } else {
+ rowVirtualizer.scrollToIndex(targetIndex, { align: "start" });
}
}, [isLoading, rowVirtualizer, hash, parsedLogs]);
const handleScrollTo = (to: "bottom" | "top") => {
- if (parsedLogs.length > 0) {
- rowVirtualizer.scrollToIndex(to === "bottom" ? parsedLogs.length - 1 :
0);
+ if (parsedLogs.length === 0) {
+ return;
+ }
+
+ const el = rowVirtualizer.scrollElement ?? parentRef.current;
+
+ if (!el) {
+ return;
}
+
+ if (to === "top") {
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(0);
+ } catch {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ } else {
+ rowVirtualizer.scrollToIndex(0, { align: "start" });
+ }
+ el.scrollTop = 0;
+ requestAnimationFrame(() => {
+ el.scrollTop = 0;
+ });
+
+ return;
+ }
+
+ // === bottom === (instant jump even for huge lists)
+ const total = rowVirtualizer.getTotalSize();
+ const clientH = el.clientHeight || 0;
+ const offset = Math.max(0, Math.floor(total - clientH));
+
+ // First tell the virtualizer where we want to be
+ if (typeof rowVirtualizer.scrollToOffset === "function") {
+ try {
+ rowVirtualizer.scrollToOffset(offset);
+ } catch {
+ rowVirtualizer.scrollToIndex(parsedLogs.length - 1, { align: "end" });
+ }
Review Comment:
This should be enough. We can remove the `typeof` check. (undefined is not
callable) (same above)
--
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]
