Skip to content

Commit 19aada7

Browse files
yottahmdyottahmd
authored
fix: save button permanently disabled when SSE connection fails (#1712) (#1713)
1 parent e8673de commit 19aada7

5 files changed

Lines changed: 239 additions & 345 deletions

File tree

‎ui/src/features/dags/components/dag-editor/DAGSpec.tsx‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ import { AppBarContext } from '../../../../contexts/AppBarContext';
1616
import { useConfig } from '../../../../contexts/ConfigContext';
1717
import { useUnsavedChanges } from '../../../../contexts/UnsavedChangesContext';
1818
import { useClient, useQuery } from '../../../../hooks/api';
19-
import { useDAGSpecWithConflictDetection } from '../../../../hooks/useDAGSpecWithConflictDetection';
19+
import { useContentEditor } from '../../../../hooks/useContentEditor';
20+
import { useDAGSSE } from '../../../../hooks/useDAGSSE';
2021
import LoadingIndicator from '../../../../ui/LoadingIndicator';
2122
import { DAGContext } from '../../contexts/DAGContext';
2223
import { DAGStepTable } from '../dag-details';
@@ -75,26 +76,13 @@ function DAGSpec({ fileName, localDags }: Props) {
7576
setCookie('flowchart', value, { path: '/' });
7677
setFlowchart(value);
7778
},
78-
[setCookie, flowchart, setFlowchart]
79+
[setCookie, setFlowchart]
7980
);
8081

81-
// SSE-based spec watching with conflict detection
82-
const {
83-
dag: sseData,
84-
isConnected,
85-
shouldUseFallback,
86-
currentValue,
87-
setCurrentValue,
88-
hasUnsavedChanges: localHasUnsavedChanges,
89-
conflict,
90-
resolveConflict,
91-
markAsSaved,
92-
} = useDAGSpecWithConflictDetection({
93-
fileName,
94-
enabled: true,
95-
});
82+
// SSE connection for real-time data
83+
const sseResult = useDAGSSE(fileName, true);
9684

97-
// Fallback to REST polling when SSE fails
85+
// Polling fallback (uses SSE state for config)
9886
const { data: pollingData, isLoading, mutate: mutateSpec } = useQuery(
9987
'/dags/{fileName}/spec',
10088
{
@@ -108,16 +96,38 @@ function DAGSpec({ fileName, localDags }: Props) {
10896
},
10997
},
11098
{
111-
revalidateIfStale: shouldUseFallback,
112-
revalidateOnFocus: shouldUseFallback,
99+
revalidateIfStale: sseResult.shouldUseFallback,
100+
revalidateOnFocus: sseResult.shouldUseFallback,
113101
revalidateOnMount: true,
114-
refreshInterval: shouldUseFallback ? 5000 : 0,
115-
isPaused: () => !shouldUseFallback && isConnected,
102+
refreshInterval: sseResult.shouldUseFallback ? 5000 : 0,
103+
isPaused: () => !sseResult.shouldUseFallback && sseResult.isConnected,
116104
}
117105
);
118106

119-
// Use SSE data when connected, fallback to polling data
120-
const data = isConnected && sseData ? sseData : pollingData;
107+
// Best available server spec — prefer SSE when connected, polling when not.
108+
// When SSE disconnects, sseResult.data retains stale values,
109+
// so only use SSE spec when actively connected.
110+
const serverSpec =
111+
sseResult.isConnected && sseResult.data?.spec != null
112+
? sseResult.data.spec
113+
: pollingData?.spec ?? null;
114+
115+
// Change tracking (source-agnostic)
116+
const {
117+
currentValue,
118+
setCurrentValue,
119+
hasUnsavedChanges: localHasUnsavedChanges,
120+
conflict,
121+
resolveConflict,
122+
markAsSaved,
123+
} = useContentEditor({
124+
key: fileName,
125+
serverContent: serverSpec,
126+
});
127+
128+
// Display data (SSE when connected, polling when not)
129+
const data =
130+
sseResult.isConnected && sseResult.data ? sseResult.data : pollingData;
121131

122132
// Sync unsaved changes context
123133
useEffect(() => {
@@ -140,7 +150,7 @@ function DAGSpec({ fileName, localDags }: Props) {
140150

141151
// Save handler function
142152
const handleSave = React.useCallback(async () => {
143-
if (!currentValue) {
153+
if (currentValue == null) {
144154
showError('No changes to save', 'Make some edits before saving.');
145155
return;
146156
}
@@ -192,6 +202,7 @@ function DAGSpec({ fileName, localDags }: Props) {
192202
remoteNode,
193203
client,
194204
saveScrollPosition,
205+
showError,
195206
showToast,
196207
markAsSaved,
197208
mutateSpec,
@@ -395,14 +406,14 @@ function DAGSpec({ fileName, localDags }: Props) {
395406
<DAGEditorWithDocs
396407
value={
397408
editable
398-
? currentValue || data.spec || ''
399-
: data.spec || ''
409+
? (currentValue ?? serverSpec ?? '')
410+
: (serverSpec ?? '')
400411
}
401412
readOnly={!editable}
402413
onChange={
403414
editable
404415
? (newValue) => {
405-
setCurrentValue(newValue || '');
416+
setCurrentValue(newValue ?? '');
406417
}
407418
: undefined
408419
}

‎ui/src/hooks/useContentEditor.ts‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
import { useCallback, useEffect, useRef, useState } from 'react';
2+
3+
interface ConflictState {
4+
hasConflict: boolean;
5+
externalContent: string | null;
6+
}
7+
8+
interface UseContentEditorOptions {
9+
/** Key for resetting state on navigation (e.g., fileName) */
10+
key: string;
11+
/** Server content from any source (SSE or polling) */
12+
serverContent: string | null;
13+
}
14+
15+
interface UseContentEditorResult {
16+
/** Current editor value. null = not yet initialized. */
17+
currentValue: string | null;
18+
setCurrentValue: (value: string) => void;
19+
hasUnsavedChanges: boolean;
20+
conflict: ConflictState;
21+
resolveConflict: (action: 'discard' | 'ignore') => void;
22+
markAsSaved: (savedContent: string) => void;
23+
}
24+
25+
/**
26+
* Generic content editor hook with conflict detection.
27+
* Decoupled from data transport — receives serverContent from any source.
28+
* Detects when content changes externally while the user is editing.
29+
*/
30+
export function useContentEditor({
31+
key,
32+
serverContent,
33+
}: UseContentEditorOptions): UseContentEditorResult {
34+
// Track local edits (null = not yet initialized)
35+
const [currentValue, setCurrentValueState] = useState<string | null>(null);
36+
37+
// Track the last known server content (for change detection)
38+
const lastServerContentRef = useRef<string | null>(null);
39+
40+
// Track if user has started editing
41+
const hasUserEditedRef = useRef<boolean>(false);
42+
43+
// Track pending save content (to ignore our own saves coming back)
44+
const pendingSaveContentRef = useRef<string | null>(null);
45+
46+
// Ref for currentValue to avoid effect re-runs on every keystroke
47+
const currentValueRef = useRef<string | null>(null);
48+
49+
// Conflict state
50+
const [conflict, setConflict] = useState<ConflictState>({
51+
hasConflict: false,
52+
externalContent: null,
53+
});
54+
55+
// Reset all state when key changes (navigating to different item)
56+
useEffect(() => {
57+
lastServerContentRef.current = null;
58+
hasUserEditedRef.current = false;
59+
pendingSaveContentRef.current = null;
60+
currentValueRef.current = null;
61+
setCurrentValueState(null);
62+
setConflict({ hasConflict: false, externalContent: null });
63+
}, [key]);
64+
65+
// Process incoming server content changes
66+
useEffect(() => {
67+
if (serverContent == null) {
68+
return;
69+
}
70+
71+
// First load - initialize everything
72+
if (lastServerContentRef.current === null) {
73+
lastServerContentRef.current = serverContent;
74+
if (!hasUserEditedRef.current) {
75+
currentValueRef.current = serverContent;
76+
setCurrentValueState(serverContent);
77+
}
78+
return;
79+
}
80+
81+
// Check if this is our own save coming back
82+
if (pendingSaveContentRef.current === serverContent) {
83+
lastServerContentRef.current = serverContent;
84+
pendingSaveContentRef.current = null;
85+
return;
86+
}
87+
88+
// Check if server content actually changed
89+
if (serverContent === lastServerContentRef.current) {
90+
return;
91+
}
92+
93+
// Server content changed externally
94+
const hasLocalChanges =
95+
hasUserEditedRef.current &&
96+
currentValueRef.current !== lastServerContentRef.current;
97+
98+
if (hasLocalChanges) {
99+
// Conflict: user has unsaved edits AND external change occurred
100+
setConflict({
101+
hasConflict: true,
102+
externalContent: serverContent,
103+
});
104+
} else {
105+
// No local edits - update silently
106+
lastServerContentRef.current = serverContent;
107+
currentValueRef.current = serverContent;
108+
setCurrentValueState(serverContent);
109+
hasUserEditedRef.current = false;
110+
}
111+
}, [serverContent]);
112+
113+
// Handle user edits
114+
const setCurrentValue = useCallback((value: string) => {
115+
hasUserEditedRef.current = true;
116+
currentValueRef.current = value;
117+
setCurrentValueState(value);
118+
}, []);
119+
120+
// Resolve conflict
121+
const resolveConflict = useCallback(
122+
(action: 'discard' | 'ignore') => {
123+
if (action === 'discard') {
124+
// Discard local changes, accept external
125+
if (conflict.externalContent !== null) {
126+
lastServerContentRef.current = conflict.externalContent;
127+
currentValueRef.current = conflict.externalContent;
128+
setCurrentValueState(conflict.externalContent);
129+
hasUserEditedRef.current = false;
130+
}
131+
} else {
132+
// Ignore external changes, keep local
133+
// Just update the server ref to prevent repeated dialogs
134+
if (conflict.externalContent !== null) {
135+
lastServerContentRef.current = conflict.externalContent;
136+
}
137+
}
138+
setConflict({ hasConflict: false, externalContent: null });
139+
},
140+
[conflict.externalContent]
141+
);
142+
143+
// Called after successful save
144+
const markAsSaved = useCallback((savedContent: string) => {
145+
pendingSaveContentRef.current = savedContent;
146+
lastServerContentRef.current = savedContent;
147+
hasUserEditedRef.current = false;
148+
}, []);
149+
150+
// Calculate unsaved changes
151+
const hasUnsavedChanges =
152+
lastServerContentRef.current !== null &&
153+
currentValue !== null &&
154+
currentValue !== lastServerContentRef.current;
155+
156+
return {
157+
currentValue,
158+
setCurrentValue,
159+
hasUnsavedChanges,
160+
conflict,
161+
resolveConflict,
162+
markAsSaved,
163+
};
164+
}

0 commit comments

Comments
 (0)