Skip to content

fix: save button permanently disabled when SSE connection fails (#1712)#1713

Merged
yohamta0 merged 3 commits into
mainfrom
1712-editor-issue
Mar 2, 2026
Merged

fix: save button permanently disabled when SSE connection fails (#1712)#1713
yohamta0 merged 3 commits into
mainfrom
1712-editor-issue

Conversation

@yohamta0

@yohamta0 yohamta0 commented Mar 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1712

Summary by CodeRabbit

  • Refactor
    • Enhanced real-time data synchronization for DAG and document editors with automatic fallback to polling when network connections encounter issues
    • Improved external conflict detection, with clearer resolution options when your content is modified by others during active editing sessions
    • Refined data synchronization pipeline to provide more consistent and reliable server communication throughout your editing workflows

@coderabbitai

coderabbitai Bot commented Mar 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors 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

Cohort / File(s) Summary
Content Editor Hook
ui/src/hooks/useContentEditor.ts
New generic hook managing content editing with server synchronization, conflict detection, and unsaved changes tracking. Provides initialization, conflict resolution (discard/ignore external changes), and save state alignment.
Removed Conflict Detection Hooks
ui/src/hooks/useDAGSpecWithConflictDetection.ts, ui/src/hooks/useDocContentWithConflictDetection.ts
Deleted specialized conflict-detection hooks that wrapped SSE sources. Logic consolidated into the new useContentEditor hook.
DAG Editor Refactor
ui/src/features/dags/components/dag-editor/DAGSpec.tsx
Replaced useDAGSpecWithConflictDetection with useDAGSSE + useContentEditor. Updated polling fallback logic, data selection to prioritize SSE when connected, and save flow with null-safe currentValue handling. Modified flowchart cookie dependency array.
Document Editor Refactor
ui/src/pages/docs/components/DocEditor.tsx
Replaced useDocContentWithConflictDetection with useDocSSE + useContentEditor. Separated server content (from SSE) from client-side editing state. Updated change handler to use nullish coalescing (val ?? '') instead of logical OR.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references the bug being fixed (save button disabled when SSE connection fails) and includes the linked issue number #1712.
Linked Issues check ✅ Passed The PR introduces useContentEditor hook and refactors data flow to use SSE with fallback to polling, ensuring save button functionality is maintained even when SSE fails [#1712].
Out of Scope Changes check ✅ Passed All changes focus on refactoring conflict detection and editor state management to support SSE with polling fallback, directly addressing the save button reliability issue [#1712].

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 1712-editor-issue

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e8673de and 59a6e99.

📒 Files selected for processing (5)
  • ui/src/features/dags/components/dag-editor/DAGSpec.tsx
  • ui/src/hooks/useContentEditor.ts
  • ui/src/hooks/useDAGSpecWithConflictDetection.ts
  • ui/src/hooks/useDocContentWithConflictDetection.ts
  • ui/src/pages/docs/components/DocEditor.tsx
💤 Files with no reviewable changes (2)
  • ui/src/hooks/useDocContentWithConflictDetection.ts
  • ui/src/hooks/useDAGSpecWithConflictDetection.ts

Comment thread ui/src/features/dags/components/dag-editor/DAGSpec.tsx
Comment thread ui/src/hooks/useContentEditor.ts
Comment thread ui/src/pages/docs/components/DocEditor.tsx
Comment thread ui/src/pages/docs/components/DocEditor.tsx Outdated
@yohamta0 yohamta0 merged commit 19aada7 into main Mar 2, 2026
2 checks passed
@yohamta0 yohamta0 deleted the 1712-editor-issue branch March 2, 2026 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Cannot save modified dags

1 participant