fix: save button permanently disabled when SSE connection fails (#1712)#1713
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors DAG and document editors to replace custom conflict-detection hooks with a unified pattern combining SSE/polling via useDAGSSE/useDocSSE and a new generic useContentEditor hook. Eliminates useDAGSpecWithConflictDetection and useDocContentWithConflictDetection, consolidating conflict detection logic into a reusable content editor hook. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/features/dags/components/dag-editor/DAGSpec.tsx`:
- Around line 83-84: The SSE subscription call useDAGSSE(fileName, true) in
DAGSpec.tsx must include the remoteNode routing parameter so events are scoped
to the currently selected node; update the call to pass the remoteNode (e.g.,
useDAGSSE(fileName, true, remoteNode)) or the appropriate selected-node variable
available in this component, and ensure any related hook signature (useDAGSSE)
is updated to accept and forward remoteNode to the backend so all SSE requests
include the remoteNode parameter.
In `@ui/src/hooks/useContentEditor.ts`:
- Around line 121-136: The conflict resolution in resolveConflict uses a truthy
check (if (conflict.externalContent)) which skips valid empty-string external
updates; change the checks to explicitly test for null/undefined (e.g.,
conflict.externalContent != null or typeof conflict.externalContent !==
'undefined') so empty '' is handled; update both the 'discard' and 'ignore'
branches to set lastServerContentRef.current, and in the 'discard' branch also
set currentValueRef.current, call setCurrentValueState(conflict.externalContent)
and set hasUserEditedRef.current = false when externalContent is not
null/undefined.
In `@ui/src/pages/docs/components/DocEditor.tsx`:
- Around line 38-53: If SSE (useDocSSE) never provides initial content
(sseResult.data), add a non-SSE fallback fetch to populate the serverContent
passed into useContentEditor: create local state (e.g., initialServerContent),
trigger an effect when sseResult.data is undefined or sseResult.error and fetch
the doc content for docPath via the REST/HTTP endpoint your app uses, set
initialServerContent with the fetched content, then call useContentEditor with
serverContent: doc?.content ?? initialServerContent ?? null so hasUnsavedChanges
can become true and save is enabled even if SSE hasn't delivered data.
- Around line 39-40: The SSE subscription in DocEditor is missing the remoteNode
routing parameter, so update the call to useDocSSE to include the current remote
node context (e.g., pass remoteNode along with docPath) so SSEs are scoped to
the selected node; locate the useDocSSE invocation and modify it to
accept/forward remoteNode (and ensure any downstream sseResult/data handling
still uses the returned value).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/hooks/useContentEditor.tsui/src/hooks/useDAGSpecWithConflictDetection.tsui/src/hooks/useDocContentWithConflictDetection.tsui/src/pages/docs/components/DocEditor.tsx
💤 Files with no reviewable changes (2)
- ui/src/hooks/useDocContentWithConflictDetection.ts
- ui/src/hooks/useDAGSpecWithConflictDetection.ts
Fixes #1712
Summary by CodeRabbit