You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "choo121600 (via GitHub)" <gi...@apache.org> on 2025/07/10 17:00:49 UTC
[PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
choo121600 opened a new pull request, #53151:
URL: https://github.com/apache/airflow/pull/53151
## Problem
- **Initial state**: Panel resize drag works correctly
- **After refresh**: Drag direction becomes reversed (dragging left expands right panel)
- **Affected views**: Both Graph and Grid DAG views
- **Root cause**: react-resizable-panels library internal state conflicts between defaultSize and autoSaveId localStorage restoration
https://github.com/user-attachments/assets/5bfec26c-b7f5-4505-9243-7ac5ca12c0c7
## Solved
The issue was caused by a mismatch between the restored layout (from localStorage) and the component's defaultSize.
To resolve this, autoSaveId has been replaced with manual sessionStorage management and conditional remounting of the PanelGroup.
https://github.com/user-attachments/assets/331cbc45-5520-4a24-a73f-fbeb467763b1
closes: https://github.com/apache/airflow/issues/53145
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #53151:
URL: https://github.com/apache/airflow/pull/53151
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #53151:
URL: https://github.com/apache/airflow/pull/53151#discussion_r2207761885
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -68,6 +76,61 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
const { onClose, onOpen, open } = useDisclosure();
const [isRightPanelCollapsed, setIsRightPanelCollapsed] = useState(false);
+ const getDefaultSizes = useCallback(
+ (): PanelSizes => (dagView === "graph" ? DEFAULT_GRAPH_SIZES : DEFAULT_GRID_SIZES),
+ [dagView],
+ );
+
+ const getSavedSizes = useCallback((): PanelSizes => {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ try {
+ const cached = sessionStorage.getItem(storageKey);
+
+ if (cached !== null && cached !== "") {
+ const parsed: unknown = JSON.parse(cached);
+
+ if (isPanelSizes(parsed)) {
+ return parsed;
+ }
+ }
+ } catch {
+ // Failed to parse saved panel sizes - fallback to default
+ }
+
+ return getDefaultSizes();
+ }, [dagId, dagView, getDefaultSizes]);
+
+ const savePanelSizes = useCallback(
+ (sizes: Array<number>) => {
+ try {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ sessionStorage.setItem(storageKey, JSON.stringify(sizes));
+ } catch {
+ // Failed to save panel sizes - continue silently
+ }
+ },
+ [dagId, dagView],
+ );
+
+ const [currentSizes, setCurrentSizes] = useState<PanelSizes>(getSavedSizes);
+
+ const [showPanel, setShowPanel] = useState(false);
+
+ useEffect(() => {
+ const newSizes = getSavedSizes();
+
+ setCurrentSizes(newSizes);
+
+ setShowPanel(false);
+ const timer = setTimeout(() => {
+ setShowPanel(true);
+ }, PANEL_RESET_DELAY);
+
+ return () => clearTimeout(timer);
+ }, [dagView, dagId, getSavedSizes]);
Review Comment:
Ideally we would like a solution without useEffect. Is there a way to do it differently ?
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -68,6 +76,61 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
const { onClose, onOpen, open } = useDisclosure();
const [isRightPanelCollapsed, setIsRightPanelCollapsed] = useState(false);
+ const getDefaultSizes = useCallback(
+ (): PanelSizes => (dagView === "graph" ? DEFAULT_GRAPH_SIZES : DEFAULT_GRID_SIZES),
+ [dagView],
+ );
+
+ const getSavedSizes = useCallback((): PanelSizes => {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ try {
+ const cached = sessionStorage.getItem(storageKey);
+
+ if (cached !== null && cached !== "") {
+ const parsed: unknown = JSON.parse(cached);
+
+ if (isPanelSizes(parsed)) {
+ return parsed;
+ }
+ }
+ } catch {
+ // Failed to parse saved panel sizes - fallback to default
+ }
Review Comment:
I don't think we can have a failure reeading that. And if we do, that's not normal and we might want to break there instead of silently returning anything.
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -68,6 +76,61 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
const { onClose, onOpen, open } = useDisclosure();
const [isRightPanelCollapsed, setIsRightPanelCollapsed] = useState(false);
+ const getDefaultSizes = useCallback(
+ (): PanelSizes => (dagView === "graph" ? DEFAULT_GRAPH_SIZES : DEFAULT_GRID_SIZES),
+ [dagView],
+ );
+
+ const getSavedSizes = useCallback((): PanelSizes => {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ try {
+ const cached = sessionStorage.getItem(storageKey);
+
+ if (cached !== null && cached !== "") {
+ const parsed: unknown = JSON.parse(cached);
+
+ if (isPanelSizes(parsed)) {
+ return parsed;
+ }
+ }
+ } catch {
+ // Failed to parse saved panel sizes - fallback to default
+ }
+
+ return getDefaultSizes();
+ }, [dagId, dagView, getDefaultSizes]);
+
+ const savePanelSizes = useCallback(
+ (sizes: Array<number>) => {
+ try {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ sessionStorage.setItem(storageKey, JSON.stringify(sizes));
+ } catch {
+ // Failed to save panel sizes - continue silently
+ }
Review Comment:
What are we trying to catch there? This shouldn't fail.
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -44,6 +44,15 @@ import { Grid } from "./Grid";
import { NavTabs } from "./NavTabs";
import { PanelButtons } from "./PanelButtons";
+const PANEL_RESET_DELAY = 10;
+const DEFAULT_GRAPH_SIZES = [70, 30] as const;
+const DEFAULT_GRID_SIZES = [20, 80] as const;
+
+type PanelSizes = readonly [number, number];
+
+const isPanelSizes = (value: unknown): value is PanelSizes =>
+ Array.isArray(value) && value.length === 2 && value.every((size) => typeof size === "number" && size > 0);
Review Comment:
Why do we need that?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3080096479
If we use autoSaveId, then the manual sessionStorage handling and the initialSizes logic become unnecessary.
Therefore, I revert to using autoSaveId as before, which should automatically handle panel size persistence correctly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #53151:
URL: https://github.com/apache/airflow/pull/53151#discussion_r2210861864
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -88,101 +100,103 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
borderRadius="full"
boxShadow="md"
cursor="pointer"
+ left={direction === "rtl" ? 0 : undefined}
onClick={() => setIsRightPanelCollapsed(false)}
position="absolute"
- right={0}
+ right={direction === "ltr" ? 0 : undefined}
size="sm"
top="50%"
zIndex={10}
>
- <FaChevronLeft />
+ {direction === "ltr" ? <FaChevronLeft /> : <FaChevronRight />}
</IconButton>
</Tooltip>
) : undefined}
- <PanelGroup autoSaveId={dagId} direction="horizontal" ref={panelGroupRef}>
- <Panel defaultSize={dagView === "graph" ? 70 : 20} minSize={6}>
- <Box height="100%" overflowY="auto" position="relative" pr={2}>
- <PanelButtons
- dagView={dagView}
- limit={limit}
- panelGroupRef={panelGroupRef}
- setDagView={setDagView}
- setLimit={setLimit}
- />
+ <PanelGroup
+ dir={direction}
+ direction="horizontal"
+ key={`${dagId}-${dagView}-${direction}`}
+ onLayout={(sizes) => {
+ sessionStorage.setItem(storageKey, JSON.stringify(sizes));
+ }}
Review Comment:
Do we still want this?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3075119812
Ok after looking into this. If we specify `dir` on PanelGroup it should solve the problem except when switching between RTL and LTR languages. In that case I think we need to force a refresh of the DetailsLayout component. But thats a lesser bug, imo
Issue: https://github.com/bvaughn/react-resizable-panels/issues/446
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3078029456
I fixed the issue using LTR/RTL support as @bbovenzi suggested🙌
It works well now! you can see it in the screen recording below:
https://github.com/user-attachments/assets/09d0bb6a-3332-45e9-85e5-b0cae0c0bba4
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3101168370
Also, if we simplify autoSaveId to the ${dagView}-${direction} format, we should apply the same logic to the asset view.
So instead of `${assetId}-${direction}`, I’ll change it to `asset-${direction}` for consistency and to avoid unnecessary separation of panel sizes per asset.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3080248863
I will also include the asset part in this PR since it has the same issue and can be fixed in the same way.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3080185623
With the current structure, if the number of Dags increase, there’s a risk that too many panel size entries will be stored in localStorage.
So, I’m wondering if including `dagId` in the autoSaveId is really the best approach.
Also, saving different panel sizes for each Dag might reduce consistency and cause confusion.
Therefore, managing autoSaveId using just the `${dagView}-${direction}` instead of `${dagId}-${dagView}-${direction}` might provide a more consistent user experience.
What do you think about this, @pierrejeambrun and @bbovenzi ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #53151:
URL: https://github.com/apache/airflow/pull/53151#discussion_r2219219177
##########
airflow-core/src/airflow/ui/src/pages/Asset/AssetLayout.tsx:
##########
@@ -94,7 +95,12 @@ export const AssetLayout = () => {
</HStack>
<ProgressBar size="xs" visibility={Boolean(isLoading) ? "visible" : "hidden"} />
<Box flex={1} minH={0}>
- <PanelGroup autoSaveId={assetId} direction="horizontal">
+ <PanelGroup
+ autoSaveId={`${assetId}-${direction}`}
+ dir={direction}
+ direction="horizontal"
+ key={`${assetId}-${direction}`}
Review Comment:
I don't think we need to update `autoSaveId` and `key` can just be left as `assetId`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3076968880
> Ok after looking into this. If we specify `dir` on PanelGroup it should solve the problem except when switching between RTL and LTR languages. In that case I think we need to force a refresh of the DetailsLayout component. But thats a lesser bug, imo
Wow, your solution is spot on!
I didn’t realize this issue was caused by LTR vs RTL!
Your fix is so much simpler than what I had in mind. Thanks a lot!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on PR #53151:
URL: https://github.com/apache/airflow/pull/53151#issuecomment-3102891528
Thanks to @bbovenzi and @pierrejeambrun, I was able to wrap this up 🙌
I was going in the wrong direction at first, but your hints and reviews really helped me get on track. Appreciate it!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "choo121600 (via GitHub)" <gi...@apache.org>.
choo121600 commented on code in PR #53151:
URL: https://github.com/apache/airflow/pull/53151#discussion_r2209958805
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -68,6 +76,61 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
const { onClose, onOpen, open } = useDisclosure();
const [isRightPanelCollapsed, setIsRightPanelCollapsed] = useState(false);
+ const getDefaultSizes = useCallback(
+ (): PanelSizes => (dagView === "graph" ? DEFAULT_GRAPH_SIZES : DEFAULT_GRID_SIZES),
+ [dagView],
+ );
+
+ const getSavedSizes = useCallback((): PanelSizes => {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ try {
+ const cached = sessionStorage.getItem(storageKey);
+
+ if (cached !== null && cached !== "") {
+ const parsed: unknown = JSON.parse(cached);
+
+ if (isPanelSizes(parsed)) {
+ return parsed;
+ }
+ }
+ } catch {
+ // Failed to parse saved panel sizes - fallback to default
+ }
+
+ return getDefaultSizes();
+ }, [dagId, dagView, getDefaultSizes]);
+
+ const savePanelSizes = useCallback(
+ (sizes: Array<number>) => {
+ try {
+ const storageKey = `panel-${dagId}-${dagView}`;
+
+ sessionStorage.setItem(storageKey, JSON.stringify(sizes));
+ } catch {
+ // Failed to save panel sizes - continue silently
+ }
+ },
+ [dagId, dagView],
+ );
+
+ const [currentSizes, setCurrentSizes] = useState<PanelSizes>(getSavedSizes);
+
+ const [showPanel, setShowPanel] = useState(false);
+
+ useEffect(() => {
+ const newSizes = getSavedSizes();
+
+ setCurrentSizes(newSizes);
+
+ setShowPanel(false);
+ const timer = setTimeout(() => {
+ setShowPanel(true);
+ }, PANEL_RESET_DELAY);
+
+ return () => clearTimeout(timer);
+ }, [dagView, dagId, getSavedSizes]);
Review Comment:
The panel size logic was more complex before because I didn’t realize it was an LTR/RTL issue.
Thanks to Bovenzi’s insight, I can remove the useEffect and simplify the code.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix reversed PanelResizeHandle drag direction after restoring from autoSaveId [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #53151:
URL: https://github.com/apache/airflow/pull/53151#discussion_r2210859558
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -53,20 +56,29 @@ type Props = {
export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => {
const { t: translate } = useTranslation();
const { dagId = "" } = useParams();
-
const { data: dag } = useDagServiceGetDag({ dagId });
const [defaultDagView] = useLocalStorage<"graph" | "grid">("default_dag_view", "grid");
- const panelGroupRef = useRef(null);
const [dagView, setDagView] = useLocalStorage<"graph" | "grid">(`dag_view-${dagId}`, defaultDagView);
const [limit, setLimit] = useLocalStorage<number>(`dag_runs_limit-${dagId}`, 10);
-
const { fitView, getZoom } = useReactFlow();
-
- const { data: warningData } = useDagWarningServiceListDagWarnings({
- dagId,
- });
+ const { data: warningData } = useDagWarningServiceListDagWarnings({ dagId });
const { onClose, onOpen, open } = useDisclosure();
const [isRightPanelCollapsed, setIsRightPanelCollapsed] = useState(false);
+ const { i18n } = useTranslation();
+ const direction = i18n.dir();
+
+ const storageKey = `panel-${dagId}-${dagView}`;
+ let initialSizes: readonly [number, number] =
+ dagView === "graph" ? DEFAULT_GRAPH_SIZES : DEFAULT_GRID_SIZES;
+ const savedSizes = sessionStorage.getItem(storageKey);
+
+ if (savedSizes !== null) {
+ const parsed: unknown = JSON.parse(savedSizes);
+
+ if (Array.isArray(parsed) && parsed.length === 2) {
+ initialSizes = parsed as [number, number];
+ }
+ }
Review Comment:
Don't think we need this anymore?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org